feat: Reassign field ids for schema #615

c-thiel · 2024-09-08T09:34:37Z

Required for TableMetadataBuilder (#587).

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

Xuanwo · 2024-09-08T17:03:13Z

crates/iceberg/src/spec/schema.rs

+    /// If not provided, it will start from 0.
+    ///
+    /// All specified aliases and identifier fields will be updated to the new field-ids.
+    pub fn with_reassigned_field_ids(mut self, start_from: Option<i32>) -> Self {


Could you please elaborate on the circumstances under which the user wants to set this? I used to think users always need to start with 0.

Xuanwo · 2024-09-08T17:05:16Z

crates/iceberg/src/spec/schema.rs

+    ///
+    /// All specified aliases and identifier fields will be updated to the new field-ids.
+    pub fn with_reassigned_field_ids(mut self, start_from: Option<i32>) -> Self {
+        self.reassign_field_ids_from = Some(start_from.unwrap_or(0));


If we always set Some(v) here, how about just using i32 for reassign_field_ids_from with default value 0?

Typically you would pass in last-column-id from the table metadata. In the case of a CREATE TABLE this will be zero, in the case of a REPLACE TABLE it will be last-column-id where all columns get a new ID and you ensure they are not used before.

@Xuanwo only doubt with the i32 is that people might input 1 as something is required - and it's shorter than Default::default().
The rust tests are quite a good example that this can happen: We have many tests where field numbering starts from 1, so I had to re-write them to be compatible with the new builder.

I am OK with the i32 as well, just wanted to bring up that a None is probably more verbose.

I also think in this case we shoud enforce user to pass an u32 rather an Option<i32>, since if this method is called, we always reassign field ids.

Also, I think this method is currently only used by TableMetadata builder? Could we make this method pub(crate) for now to avoid exposing to user?

Both good points, fixed in the latest commit.
Its dead_code for now

Xuanwo · 2024-09-10T05:55:54Z

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

liurenjie1024 · 2024-09-25T03:26:37Z

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

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

liurenjie1024

Thanks @c-thiel for this pr!

liurenjie1024 · 2024-09-25T03:40:52Z

crates/iceberg/src/spec/schema.rs

+    ///
+    /// All specified aliases and identifier fields will be updated to the new field-ids.
+    pub fn with_reassigned_field_ids(mut self, start_from: Option<i32>) -> Self {
+        self.reassign_field_ids_from = Some(start_from.unwrap_or(0));


I also think in this case we shoud enforce user to pass an u32 rather an Option<i32>, since if this method is called, we always reassign field ids.

liurenjie1024 · 2024-09-25T03:42:07Z

crates/iceberg/src/spec/schema.rs

+    ///
+    /// All specified aliases and identifier fields will be updated to the new field-ids.
+    pub fn with_reassigned_field_ids(mut self, start_from: Option<i32>) -> Self {
+        self.reassign_field_ids_from = Some(start_from.unwrap_or(0));


Also, I think this method is currently only used by TableMetadata builder? Could we make this method pub(crate) for now to avoid exposing to user?

liurenjie1024 · 2024-09-25T03:44:19Z

crates/iceberg/src/spec/schema.rs

+        let outer_fields = fields
+            .into_iter()
+            .map(|field| {
+                self.old_to_new_id.insert(field.id, self.next_field_id);


We should check exisistence of field.id here since we don't trust user provide field ids, and it may duplicate.

I added the check. It should never trigger currently, because I switched the build method order as you suggested.
Nevertheless the check is good if Reassignment is ever used outside of the builder in the future.

liurenjie1024 · 2024-09-25T03:50:14Z

crates/iceberg/src/spec/schema.rs

@@ -105,7 +116,16 @@ impl SchemaBuilder {
    }

    /// Builds the schema.
-    pub fn build(self) -> Result<Schema> {
+    pub fn build(mut self) -> Result<Schema> {
+        if let Some(start_from) = self.reassign_field_ids_from {


I'm thinking that maybe we should put this part in the end of this method? The build process has some check, for example identifier field type, uniqueness of field name. Though current approach is correct, the error reporting maybe confusing. For example when some field doesn't pass identifier type check, current approach reports reassigned id, rather original id.

I agree - I didn't think about the error messages.
It's unfortunately not super straightforward to switch the order, as for example index_by_id needs a full rebuild as both keys and values changes. I think we have two options to achieve the swap:

Use id_reassigner.apply_* for all other fields that have been build (i.e. id_to_field) or

Re-build those fields using the same code - i.e. the build option.

I opted for 2.
While it is slightly less performant because we have to visit the structure again, it is much less error prone as we simply re-use existing and well-tested code.

Let me know what you think!

I'm also opted for 2, which is much cleaner and easier to maintain. About performance, I don't think this is a concern for now as it's not supposed to be called frequently. Current codes are working as opt 1, are you planning to refactoring it in this pr to opt 2? BTW currently opt 1 also looks good to me, so it's up to you to do it in this pr or future pr.

@liurenjie1024 I refactored it in my latest commit, however we cannot get rid of the two functions:
apply_to_aliases and apply_to_identifier_fields. The builder itself is standard now.

liurenjie1024 · 2024-09-25T03:52:40Z

crates/iceberg/src/spec/schema.rs

@@ -2237,4 +2377,168 @@ table {
        let schema = table_schema_simple().0;
        assert_eq!(3, schema.highest_field_id());
    }
+
+    #[test]


Thanks for the tests!

c-thiel · 2024-09-25T11:17:37Z

@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 · 2024-09-25T11:59:54Z

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 :)

liurenjie1024

Thanks @c-thiel for this great pr!

liurenjie1024 · 2024-09-30T03:10:27Z

crates/iceberg/src/spec/schema.rs

@@ -86,6 +87,16 @@ impl SchemaBuilder {
        self
    }

+    /// Reassign all field-ids (nested) on build.


Suggested change

/// Reassign all field-ids (nested) on build.

/// Reassign all field-ids (including nested) on build.

Fixed in latest commit

liurenjie1024 · 2024-09-30T03:28:03Z

crates/iceberg/src/spec/schema.rs

@@ -105,7 +116,16 @@ impl SchemaBuilder {
    }

    /// Builds the schema.
-    pub fn build(self) -> Result<Schema> {
+    pub fn build(mut self) -> Result<Schema> {
+        if let Some(start_from) = self.reassign_field_ids_from {


I'm also opted for 2, which is much cleaner and easier to maintain. About performance, I don't think this is a concern for now as it's not supposed to be called frequently. Current codes are working as opt 1, are you planning to refactoring it in this pr to opt 2? BTW currently opt 1 also looks good to me, so it's up to you to do it in this pr or future pr.

c-thiel · 2024-10-01T07:07:15Z

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

liurenjie1024 · 2024-10-04T03:22:25Z

Thanks @c-thiel for this pr!

* Reassign field ids for schema * Address comments * Schema ensure unique field ids * Fix tests with duplicate nested field ids * Use Schema::builder() for reassigned ids * Better docs

Reassign field ids for schema

1eb6950

c-thiel changed the title ~~Feat: Reassign field ids for schema~~ feat: Reassign field ids for schema Sep 8, 2024

c-thiel mentioned this pull request Sep 8, 2024

TableMetadataBuilder #587

Merged

Xuanwo reviewed Sep 8, 2024

View reviewed changes

Merge branch 'main' into feat/schema-reassign-field-ids

a0deaae

liurenjie1024 reviewed Sep 25, 2024

View reviewed changes

Address comments

0bbe868

c-thiel added 2 commits September 25, 2024 13:18

Schema ensure unique field ids

8009a46

Fix tests with duplicate nested field ids

efbd436

liurenjie1024 approved these changes Sep 30, 2024

View reviewed changes

c-thiel added 2 commits October 1, 2024 08:58

Use Schema::builder() for reassigned ids

15b5455

Better docs

4b2eb54

liurenjie1024 merged commit 2bc66c4 into apache:main Oct 4, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Reassign field ids for schema #615

feat: Reassign field ids for schema #615

c-thiel commented Sep 8, 2024

Xuanwo Sep 8, 2024

Xuanwo Sep 8, 2024

Fokko Sep 8, 2024

c-thiel Sep 9, 2024

liurenjie1024 Sep 25, 2024

liurenjie1024 Sep 25, 2024

c-thiel Sep 25, 2024

Xuanwo commented Sep 10, 2024

liurenjie1024 commented Sep 25, 2024

liurenjie1024 left a comment

liurenjie1024 Sep 25, 2024

liurenjie1024 Sep 25, 2024

liurenjie1024 Sep 25, 2024

c-thiel Sep 25, 2024

liurenjie1024 Sep 25, 2024

c-thiel Sep 25, 2024

liurenjie1024 Sep 30, 2024

c-thiel Oct 1, 2024

liurenjie1024 Sep 25, 2024

c-thiel commented Sep 25, 2024

c-thiel commented Sep 25, 2024

liurenjie1024 left a comment

liurenjie1024 Sep 30, 2024

c-thiel Oct 1, 2024

liurenjie1024 Sep 30, 2024

c-thiel commented Oct 1, 2024

liurenjie1024 commented Oct 4, 2024

	/// Reassign all field-ids (nested) on build.
	/// Reassign all field-ids (including nested) on build.

feat: Reassign field ids for schema #615

feat: Reassign field ids for schema #615

Conversation

c-thiel commented Sep 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Sep 10, 2024

liurenjie1024 commented Sep 25, 2024

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-thiel commented Sep 25, 2024

c-thiel commented Sep 25, 2024

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-thiel commented Oct 1, 2024

liurenjie1024 commented Oct 4, 2024