feat: add update table properties support #363
Conversation
WZhuo
force-pushed
the
update_properties
branch
23 times, most recently
from
5f98124 to
387401a
Compare
WZhuo
force-pushed
the
update_properties
branch
5 times, most recently
from
11604e3 to
ce0b6ea
Compare
WZhuo
force-pushed
the
update_properties
branch
from
ce0b6ea to
d50ffd6
Compare
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
This looks good now. I just left some minor comments.
| "counts"; | ||
|
|
||
| auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_); | ||
| EXPECT_FALSE(result.has_value()) |
There was a problem hiding this comment.
nit: you can #include "iceberg/test/matchers.h" and then write
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
EXPECT_THAT(result, HasErrorMessage("..."));
There was a problem hiding this comment.
Replace all EXPECT_xxx(result.has_value()) with EXPECT_THAT(result, IsError/IsOk);
| UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog, | ||
| std::shared_ptr<TableMetadata> metadata); |
There was a problem hiding this comment.
Sounds good. I still suggest renaming metadata to base to make it explicit.
Do we need to check if catalog or base is null? We cannot throw in the ctor, perhaps we should check them in Apply and Commit.
| /// | ||
| /// \param format The file format type to use as default | ||
| /// \return Reference to this UpdateProperties for chaining | ||
| UpdateProperties& DefaultFormat(FileFormatType format); |
There was a problem hiding this comment.
Yes I understand. I still think we don't need special case the file format config to make the API simple. The Java impl seems have some historical reasons and we don't need to follow everything.
WZhuo
force-pushed
the
update_properties
branch
from
d50ffd6 to
2b308fb
Compare
WZhuo
force-pushed
the
update_properties
branch
from
2b308fb to
04505a4
Compare
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for working on this! LGTM
wgtmac
changed the title
3 participants
No description provided.