feat: implement RewriteFiles operation#751
Conversation
WZhuo
force-pushed
the
rewrite_files
branch
2 times, most recently
from
0acf4f8 to
d28ffe9
Compare
| // ============================================================================ | ||
| // | ||
| // TODO(RewriteDataAndDeleteFiles): | ||
| // Blocked by: RowDelta not yet ported. |
There was a problem hiding this comment.
Just FYI, RowDelta has been merged.
| /// \param table_name The name of the table | ||
| /// \param ctx The transaction context | ||
| /// \return A unique pointer to the new RewriteFiles operation | ||
| static Result<std::unique_ptr<RewriteFiles>> Make( |
There was a problem hiding this comment.
I noticed that some update operations return a std::unique_ptr (e.g., RowDelta), while others return a std::shared_ptr (e.g., OverwriteFiles). I'm not sure what the standard is here. Should we make them consistent? cc @wgtmac
|
|
||
| namespace iceberg { | ||
|
|
||
| class RewriteFilesTest : public MinimalUpdateTestBase { |
There was a problem hiding this comment.
Should we consider the table format version in these tests? I handled it in MergeAppendTestBase, but the resulting code structure feels a bit awkward. I'm wondering if we should refactor UpdateTestBase to make it easier to write tests for different table format versions.
| // Verifies sequence number propagation to new manifests. | ||
| // | ||
| // TODO(Failure): | ||
| // Blocked by: commit fail injection (failCommits) not yet ported. |
There was a problem hiding this comment.
I think this is already supported. See BindTableWithFailingCommits in merge_append_test.cc. The implementation is a bit ugly though, feel free to improve it.
3 participants
This PR implements the RewriteFiles operation for the Iceberg C++ library.
Changes