search

apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
673 stars 159 forks source link

feat: Reassign field ids for schema #615

Closed c-thiel closed 1 month ago

c-thiel commented 2 months ago

Required for TableMetadataBuilder (https://github.com/apache/iceberg-rust/pull/587).

When new TableMetadata is created, field ids should be reassign to ensure that field numbering is normalized and started from 0.

Xuanwo commented 2 months ago

cc @liurenjie1024 would you like to take a look too?

liurenjie1024 commented 1 month ago

cc @liurenjie1024 would you like to take a look too?

Thanks for pinging me, I'll take a review.

c-thiel commented 1 month ago

@liurenjie1024 I addressed all of your comments. I also noticed that I didn't handle a potential field_id overflow error which I also fixed.

Regarding your uniqueness of id comment: I noticed that this wasn't enforced previously, so I fixed it and added a test for it as well. This is currently making a few tests crash, where key-ids & value-ids of maps are re-used elsewhere in the schema.

I am checking how java handles this and will fix the tests accordingly.

c-thiel commented 1 month ago

Ok, java does not allow this. It's easy to test with the testNestedStructSchemaToMapping java test. I fixed the rusts test that failed due to duplicate nested IDs.

@liurenjie1024 ready for another round :)

c-thiel commented 1 month ago

@liurenjie1024 last two minor changes added - I think we are good to merge @liurenjie1024 @Xuanwo

liurenjie1024 commented 1 month ago

Thanks @c-thiel for this pr!