TableMetadataBuilder

[c-thiel]

This PR is now ready for first reviews. Some Remarks:

• In add_sort_order and add_partition_spec the Java code re-builds the added sort-order against the current schema by matching column names. This implementation currently does not do this. Adding this feature would require PartitionSpec (bound) to store the schema it was bound against (probably a good idea anyway) and split SortOrder in bound and unbound, where the bound SortOrder also stores the schema it was bound against. Instead, this implementation assumes that provided sort-orders and partition-specs are valid for the current schema. Compatibility with the current schema is tested.
• In contrast to java, our add_schema method does not require a new_last_column_id argument. In java there is a todo to achieve the same. I put my reasoning in a comment in the code, feel free to comment on it.
• I have a few specifics that would be good to discuss with a Java Maintainer with a Java Maintainer. Its not ready to be merged yet - please wait for my OK too :)
• I had to change a few tests that used the builder under the hood - mostly due to the changed new() behaviour that now re-assignes field-ids to start from 0. Some tests started from 1 before. Re-assigning the ids, just like in Java, ensures that fresh metadata always has fresh and correct ids even if they are created manually or re-used from another metadata.

[c-thiel]

ToDos:

• [x] First PR as discussion basis
• [x] Reserved Property handling
• [ ] Finish discussion about SortOrder and PartitionSpec re-binding
• [ ] Review from 1-2 Java Maintainers
• [ ] Review from 1-2 Rust Maintainers

[c-thiel]

Fixes https://github.com/apache/iceberg-rust/issues/232

[liurenjie1024]

Thanks @c-thiel for this pr, I've skimmed through it and it looks great to me. However this pr is too huge to review(3k lines), would you mind to split them into smaller onces? For example, we can add one pr for methods involved in one TableUpdate action and add enough tests for it? Also it would be better to put refactoring TableMetadataBuilder in a standalone module a pr?

[c-thiel]

Thanks for your Feedback @liurenjie1024. This isn't really a refactoring of the builder, it's more a complete rewrite. The old builder allowed to create corrupt metadata in various ways. Splitting it up by TableUpdate would not be straight forward - many tests also in other modules depend on building Metadata. Creating Metadata now always goes through builder methods - it's a different architecture that requires basic support for all methods from the beginning just to keep tests running.

I would currently prefer to keep it as a larger block mainly because:

• I don't have much time currently and its going to be more effort
• We would need to write auxiliary code to provide non-checked methods so that crate tests don't fail
• The total timespan of merging 10 or so PRs is expected to be much larger than putting ~2 full days effort in from a Rust Maintainer and a Java Maintainer to review it as a block.
• Patterns are repetitive and can be reviewed together in many cases
• A lot of it are tests - the core builder are 1145 lines. Its long - but doable :)

We now have a vision of what it could look like in the end. Before putting any more effort in, we should answer the following questions:

• Is the overall structure OK or should we head in a different direction?
• My first point of the opening statement: Do we re-write our SortOrder and add the schema to PartitionSpec so that we can match on names like Java does or not?
• My second point from the opening statement: How do we handle new_last_column_id

Those points might change the overall design quite a bit and might require a re-write of SortOrder first (split to bound and unbound).

After we answered those questions, and we still think splitting makes sense, I can try to find time to build stacked-PRs. Maybe just splitting normalization / validation in table_metadata.rs from the actual builder would be a leaner option than splitting every single TableUpdate?

[c-thiel]

@liurenjie1024 I tried to cut a few things out - but not along the lines of TalbeUpdate. I hope that's OK?

1. https://github.com/apache/iceberg-rust/pull/611
2. https://github.com/apache/iceberg-rust/pull/612
3. https://github.com/apache/iceberg-rust/pull/613
4. https://github.com/apache/iceberg-rust/pull/614
5. https://github.com/apache/iceberg-rust/pull/615

After they are all merged, I'll rebase this PR for the actual builder.

[liurenjie1024]

Hi, @c-thiel Sorry for late reply.

Is the overall structure OK or should we head in a different direction?

I've went through the new builder and I think this is your design is the right direction.

My first point of the opening statement: Do we re-write our SortOrder and add the schema to PartitionSpec so that we can match on names like Java does or not?

To be honest, I don't quite understand the use case. We can ask for background of this in dev channel, but I think this is not a blocker of this pr, we can always add this later.

My second point from the opening statement: How do we handle new_last_column_id

I've took a look at the comments of these two prs: https://github.com/apache/iceberg/pull/6701 https://github.com/apache/iceberg/pull/7445

And I think the reason behavior is the last_column_id is optional, and we calculate it from highest field id when missing. But allowing user to pass last_column_id should be added to be compatible with current behavior, but this should be a feature which could be added later.

[liurenjie1024]

Those points might change the overall design quite a bit and might require a re-write of SortOrder first (split to bound and unbound).

I agree that this should be required, as I mentioned in #550

[liurenjie1024]

After we answered those questions, and we still think splitting makes sense, I can try to find time to build stacked-PRs. Maybe just splitting normalization / validation in table_metadata.rs from the actual builder would be a leaner option than splitting every single TableUpdate?

That sound reasonable to me. If one pr per table update is too much burden, could we split them by components, for example sort oder, partition spec, schema changes?

[c-thiel]

@liurenjie1024 thanks for the Feedback!

My first point of the opening statement: Do we re-write our SortOrder and add the schema to PartitionSpec so that we can match on names like Java does or not?

To be honest, I don't quite understand the use case. We can ask for background of this in dev channel, but I think this is not a blocker of this pr, we can always add this later.

The problem in changing it later is that it changes the semantic of the function. Right now we expect source_id to match the current_schema() (which is also the reason why I expose it during build). Java doesn't do this, instead, it looks up the name by id in the schema bound to a SortOrder or PartitionSpec, and then searches for the same name in the new schema to use it.

In my opinion ids are much cleaner than names (we might have dropped and re-added a column with the same name in the meantime), so I am OK with going forward. However, moving over to java semantics will require new endpoints (i.e. add_migrate_partition_spec or so), which takes a bound partition spec in contrast to the unbound spec we currently pass in.

Give me a thumbs up if that's OK for you. I'll also open a discussion in the dev channel to get some more opinions.

[c-thiel]

My second point from the opening statement: How do we handle new_last_column_id

I've took a look at the comments of these two prs: https://github.com/apache/iceberg/pull/6701 https://github.com/apache/iceberg/pull/7445

And I think the reason behavior is the last_column_id is optional, and we calculate it from highest field id when missing. But allowing user to pass last_column_id should be added to be compatible with current behavior, but this should be a feature which could be added later.

I don't think we should add the argument to be honest. My reasoning is as follows: If specified, it could be a way to artificially increase last_assigned_field_id of a TableMetadata. I can't see any motivation behind that. Its just adds a source of confusion of what to specify here - and what to do if its wrong. The only useful thing to do with it is to check for outdated TableMetadata at the time of constructing the Schema. I added this check here: https://github.com/apache/iceberg-rust/pull/587/files#diff-04f26c83b3c614be6f6d6cfb6c4cefef9e01ec2d31395ac487cdcdff2dbae729R442-R451

Maybe add @nastra or @Fokko could add some comments on the intention of that parameter?

[Xuanwo]

I have reviewed most PRs that I am confident can be merged. The only one left is https://github.com/apache/iceberg-rust/pull/615, for which I need more input.

[c-thiel]

@Xuanwo, @liurenjie1024 this PR is ready for another round of review. It's now rebased on the 6 PRs we merged during the last months. The core logic is ~1100 lines of code, including quite a bit of comments. Its not very straightforward to split it up further, as even the new function already requires a lot of the overall logic.

[c-thiel]

@liurenjie1024 for set_branch_snapshot I was missing an easy way to determine the type of a ref. I introduced an enum ReferenceType, but if you don't like that, I can also just implement is_branch for SnapshotReference.

[c-thiel]

@liurenjie1024 for set_branch_snapshot I was missing an easy way to determine the type of a ref. I introduced an enum ReferenceType, but if you don't like that, I can also just implement is_branch for SnapshotReference.

Slept over it and ReferenceType felt wrong - removed it.