You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "c-thiel (via GitHub)" <gi...@apache.org> on 2024/09/08 09:34:40 UTC
[PR] Feat: Reassign field ids for schema [iceberg-rust]
c-thiel opened a new pull request, #615:
URL: https://github.com/apache/iceberg-rust/pull/615
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1749940779
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1774977970
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -943,6 +965,122 @@ impl SchemaVisitor for PruneColumn {
}
}
+struct ReassignFieldIds {
+ next_field_id: i32,
+ old_to_new_id: HashMap<i32, i32>,
+}
+
+// We are not using the visitor here, as post order traversal is not desired.
+// Instead we want to re-assign all fields on one level first before diving deeper.
+impl ReassignFieldIds {
+ fn new(start_from: i32) -> Self {
+ Self {
+ next_field_id: start_from,
+ old_to_new_id: HashMap::new(),
+ }
+ }
+
+ fn reassign_field_ids(&mut self, fields: Vec<NestedFieldRef>) -> Vec<NestedFieldRef> {
+ // Visit fields on the same level first
+ let outer_fields = fields
+ .into_iter()
+ .map(|field| {
+ self.old_to_new_id.insert(field.id, self.next_field_id);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#issuecomment-2373795823
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#issuecomment-2339690333
cc @liurenjie1024 would you like to take a look too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1749313279
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#issuecomment-2373882838
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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1774372777
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -943,6 +965,122 @@ impl SchemaVisitor for PruneColumn {
}
}
+struct ReassignFieldIds {
+ next_field_id: i32,
+ old_to_new_id: HashMap<i32, i32>,
+}
+
+// We are not using the visitor here, as post order traversal is not desired.
+// Instead we want to re-assign all fields on one level first before diving deeper.
+impl ReassignFieldIds {
+ fn new(start_from: i32) -> Self {
+ Self {
+ next_field_id: start_from,
+ old_to_new_id: HashMap::new(),
+ }
+ }
+
+ fn reassign_field_ids(&mut self, fields: Vec<NestedFieldRef>) -> Vec<NestedFieldRef> {
+ // Visit fields on the same level first
+ let outer_fields = fields
+ .into_iter()
+ .map(|field| {
+ self.old_to_new_id.insert(field.id, self.next_field_id);
Review Comment:
We should check exisistence of `field.id` here since we don't trust user provide field ids, and it may duplicate.
##########
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 {
Review Comment:
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.
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
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.
##########
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]
Review Comment:
Thanks for the tests!
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1774983362
##########
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 {
Review Comment:
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:
1. Use `id_reassigner.apply_*` for all other fields that have been build (i.e. `id_to_field`) or
2. 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#issuecomment-2372832599
> cc @liurenjie1024 would you like to take a look too?
Thanks for pinging me, I'll take a review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1749297098
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
Review Comment:
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`.
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
If we always set `Some(v)` here, how about just using `i32` for `reassign_field_ids_from` with default value `0`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#issuecomment-3059287463
> When new TableMetadata is created, field ids should be reassign to ensure that field numbering is normalized and started from 0.
I'm not sure if I mis-read, but it seems field id starts with 1:
- https://github.com/apache/iceberg-rust/blob/3fa902789dae510acb0bd3e9e57e9f7b92a0677d/crates/iceberg/src/spec/table_metadata_builder.rs#L34
- every time we create a new `TableMetadata`, we reassign field id for normalization, but the field id somehow starts with 1 but not 0: https://github.com/apache/iceberg-rust/blob/3fa902789dae510acb0bd3e9e57e9f7b92a0677d/crates/iceberg/src/spec/table_metadata_builder.rs#L89
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Reassign field ids for schema [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #615:
URL: https://github.com/apache/iceberg-rust/pull/615#discussion_r1774975537
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -86,6 +87,16 @@ impl SchemaBuilder {
self
}
+ /// Reassign all field-ids (nested) on build.
+ /// If `start_from` is provided, it will start reassigning from that id (inclusive).
+ /// 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 {
+ self.reassign_field_ids_from = Some(start_from.unwrap_or(0));
Review Comment:
Both good points, fixed in the latest commit.
Its `dead_code` for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org