laskoviymishka commented on code in PR #3066: URL: https://github.com/apache/iceberg-rust/pull/3066#discussion_r4108362092
########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { Review Comment: The contract on `start_from` is the load-bearing thing here, and right now it's neither documented nor enforced. `id_for` only advances `next_field_id` on a miss, so a reused base id never consumes a slot — which means `start_from` has to sit past every id reachable from `base`, not just past `schema`'s own ids. With `base = {"a": 5}`, replacement `[a, b]`, `start_from = 5`, `a` reuses 5 and `b` also gets 5; the only thing that catches it is the generic duplicate-id check in `build()` ("Found duplicate 'field.id' 5"), which gives zero hint the real cause is `start_from` being too low. The subtler hazard is what #3056 will seed this with. Java's `TableMetadata.buildReplacement` seeds from the table-wide `lastColumnId` — monotonic, and it reserves the ids of columns already dropped from the current schema — not from any single schema's `highest_field_id()`. If the follow-up seeds `start_from` from `updated_schema.highest_field_id()+1`, a brand-new column can be handed the id of a column that was dropped from the current schema: that id is absent from both `base` and the new schema, so nothing here or in `build()` fires, and we get spec-violating id reuse that corrupts historical reads and manifests keyed on that field id. The correct seed is `table_metadata.last_column_id()+1` — the sibling `UpdateSchema` already does exactly this (`update_schema.rs:338`). I'd document the precondition on the fn plainly (callers must pass `table_metadata.last_column_id()+1`, never a schema's `highest_field_id()`) and add a `debug_assert!(start_from > base.highest_field_id())` as a cheap guard. While you're there, a short rustdoc covering the reuse-by-name behavior and the roles of `schema`/`base`/`start_from` would help — none of it is derivable from the signature today. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) Review Comment: We thread the incoming `schema_id` straight through, but Java's 3-arg `assignFreshIds` never propagates it — the real schema-id is arbitrated later in `addSchemaInternal`, whose equivalent here is `TableMetadataBuilder::add_schema` (which documents that the provided `schema_id` may not be used). As long as #3056 routes the output through `add_schema` this is inert, but the test asserting `schema_id() == 1` treats the preserved id as meaningful, which could nudge a caller into persisting it directly. I'd note in the rustdoc that the returned `schema_id` isn't authoritative and must be re-arbitrated via `add_schema`, and relax that assertion. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) + .with_fields(fields) + .with_identifier_field_ids(identifier_field_ids) + .with_alias(alias_to_id) + .build() +} + +struct AssignFreshIds { + next_field_id: i32, + target_names: HashMap<i32, String>, + base_ids: HashMap<String, i32>, + old_to_new_id: HashMap<i32, i32>, +} + +impl AssignFreshIds { + fn new(target: &Schema, base: &Schema, start_from: i32) -> Self { + Self { + next_field_id: start_from, + target_names: target.field_id_to_name_map().clone(), + base_ids: base + .field_id_to_name_map() + .iter() + .map(|(id, name)| (name.clone(), *id)) + .collect(), + old_to_new_id: HashMap::new(), + } + } + + fn id_for(&mut self, old_id: i32) -> Result<i32> { Review Comment: `id_for` reads like a pure getter but it advances `next_field_id` on a miss — unlike `id_reassigner`'s visibly separate `increase_next_field_id()` step. A reader tracing how many ids get consumed can easily miscount. I'd rename it to something like `resolve_or_assign_id`, or drop a one-line doc noting the side effect. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) + .with_fields(fields) + .with_identifier_field_ids(identifier_field_ids) + .with_alias(alias_to_id) + .build() +} + +struct AssignFreshIds { + next_field_id: i32, + target_names: HashMap<i32, String>, + base_ids: HashMap<String, i32>, + old_to_new_id: HashMap<i32, i32>, +} + +impl AssignFreshIds { + fn new(target: &Schema, base: &Schema, start_from: i32) -> Self { + Self { + next_field_id: start_from, + target_names: target.field_id_to_name_map().clone(), + base_ids: base + .field_id_to_name_map() + .iter() + .map(|(id, name)| (name.clone(), *id)) + .collect(), + old_to_new_id: HashMap::new(), + } + } + + fn id_for(&mut self, old_id: i32) -> Result<i32> { + if let Some(id) = self + .target_names + .get(&old_id) + .and_then(|name| self.base_ids.get(name)) + { + return Ok(*id); + } + + let id = self.next_field_id; + self.next_field_id = self.next_field_id.checked_add(1).ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + "Field ID overflowed, cannot add more fields", + ) + })?; + Ok(id) + } + + fn assign_fields(&mut self, fields: Vec<NestedFieldRef>) -> Result<Vec<NestedFieldRef>> { + let outer_fields = fields + .into_iter() + .map(|field| { + let new_id = self.id_for(field.id)?; + try_insert_field(&mut self.old_to_new_id, field.id, new_id)?; + Ok(Arc::new(Arc::unwrap_or_clone(field).with_id(new_id))) + }) + .collect::<Result<Vec<_>>>()?; + + outer_fields + .into_iter() + .map(|field| { + if field.field_type.is_primitive() { + Ok(field) + } else { + let mut field = Arc::unwrap_or_clone(field); + *field.field_type = self.assign_type(*field.field_type)?; + Ok(Arc::new(field)) + } + }) + .collect() + } + + fn assign_type(&mut self, field_type: Type) -> Result<Type> { + match field_type { + Type::Primitive(primitive) => Ok(Type::Primitive(primitive)), + Type::Struct(r#struct) => Ok(Type::Struct(StructType::new( + self.assign_fields(r#struct.fields().to_vec())?, + ))), + Type::List(list) => { + let new_id = self.id_for(list.element_field.id)?; + try_insert_field(&mut self.old_to_new_id, list.element_field.id, new_id)?; + let mut element_field = Arc::unwrap_or_clone(list.element_field); + element_field.id = new_id; + *element_field.field_type = self.assign_type(*element_field.field_type)?; + Ok(Type::List(ListType { + element_field: Arc::new(element_field), + })) + } + Type::Map(map) => { Review Comment: This resolves `new_key_id` and `new_value_id` together before recursing into either subtree, whereas `ReassignFieldIds` assigns the key's id and fully recurses the key before touching the value. Both orders are deliberate — this one matches Java's key-then-value-then-recurse, and `test_assign_fresh_ids_assigns_map_ids_before_nested_types` pins it — but the divergence from the near-identical sibling is silent. A one-line comment on this arm saying key and value ids are resolved together before recursing, intentionally unlike `ReassignFieldIds`, would keep someone from "harmonizing" the two and quietly breaking the ordering the test enforces. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) + .with_fields(fields) + .with_identifier_field_ids(identifier_field_ids) + .with_alias(alias_to_id) + .build() +} + +struct AssignFreshIds { + next_field_id: i32, + target_names: HashMap<i32, String>, + base_ids: HashMap<String, i32>, + old_to_new_id: HashMap<i32, i32>, +} + +impl AssignFreshIds { + fn new(target: &Schema, base: &Schema, start_from: i32) -> Self { + Self { + next_field_id: start_from, + target_names: target.field_id_to_name_map().clone(), + base_ids: base + .field_id_to_name_map() + .iter() + .map(|(id, name)| (name.clone(), *id)) + .collect(), + old_to_new_id: HashMap::new(), + } + } + + fn id_for(&mut self, old_id: i32) -> Result<i32> { + if let Some(id) = self + .target_names + .get(&old_id) + .and_then(|name| self.base_ids.get(name)) + { + return Ok(*id); + } + + let id = self.next_field_id; + self.next_field_id = self.next_field_id.checked_add(1).ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + "Field ID overflowed, cannot add more fields", + ) + })?; + Ok(id) + } + + fn assign_fields(&mut self, fields: Vec<NestedFieldRef>) -> Result<Vec<NestedFieldRef>> { + let outer_fields = fields + .into_iter() + .map(|field| { + let new_id = self.id_for(field.id)?; + try_insert_field(&mut self.old_to_new_id, field.id, new_id)?; + Ok(Arc::new(Arc::unwrap_or_clone(field).with_id(new_id))) + }) + .collect::<Result<Vec<_>>>()?; + + outer_fields + .into_iter() + .map(|field| { + if field.field_type.is_primitive() { + Ok(field) + } else { + let mut field = Arc::unwrap_or_clone(field); + *field.field_type = self.assign_type(*field.field_type)?; + Ok(Arc::new(field)) + } + }) + .collect() + } + + fn assign_type(&mut self, field_type: Type) -> Result<Type> { + match field_type { + Type::Primitive(primitive) => Ok(Type::Primitive(primitive)), + Type::Struct(r#struct) => Ok(Type::Struct(StructType::new( + self.assign_fields(r#struct.fields().to_vec())?, + ))), + Type::List(list) => { + let new_id = self.id_for(list.element_field.id)?; + try_insert_field(&mut self.old_to_new_id, list.element_field.id, new_id)?; + let mut element_field = Arc::unwrap_or_clone(list.element_field); + element_field.id = new_id; + *element_field.field_type = self.assign_type(*element_field.field_type)?; + Ok(Type::List(ListType { + element_field: Arc::new(element_field), + })) + } + Type::Map(map) => { + let new_key_id = self.id_for(map.key_field.id)?; + let new_value_id = self.id_for(map.value_field.id)?; + try_insert_field(&mut self.old_to_new_id, map.key_field.id, new_key_id)?; + try_insert_field(&mut self.old_to_new_id, map.value_field.id, new_value_id)?; + + let mut key_field = Arc::unwrap_or_clone(map.key_field); + key_field.id = new_key_id; + *key_field.field_type = self.assign_type(*key_field.field_type)?; + + let mut value_field = Arc::unwrap_or_clone(map.value_field); + value_field.id = new_value_id; + *value_field.field_type = self.assign_type(*value_field.field_type)?; + + Ok(Type::Map(MapType { + key_field: Arc::new(key_field), + value_field: Arc::new(value_field), + })) + } + Type::Variant(variant) => Ok(Type::Variant(variant)), + } + } + + fn apply_to_identifier_fields(&self, field_ids: HashSet<i32>) -> Result<HashSet<i32>> { + field_ids + .into_iter() + .map(|id| { + self.old_to_new_id.get(&id).copied().ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("Identifier Field ID {id} not found"), + ) + }) + }) + .collect() + } + + fn apply_to_aliases(&self, aliases: BiHashMap<String, i32>) -> Result<BiHashMap<String, i32>> { Review Comment: Unlike `identifier_field_ids`, which `build()` validates via `validate_identifier_ids`, `alias_to_id` has no equivalent validation — so this "not found" branch is genuinely reachable: an alias pointing at a field dropped in the replacement schema lands right here, and it's untested. I'd add a small regression test that feeds an alias for a dropped field and asserts the error, so the branch is pinned. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) + .with_fields(fields) + .with_identifier_field_ids(identifier_field_ids) + .with_alias(alias_to_id) + .build() +} + +struct AssignFreshIds { Review Comment: This is the third near-identical copy of the field-tree id walker — `ReassignFieldIds` (`id_reassigner.rs`) and the private one in `transaction/update_schema.rs` are the other two, and `apply_to_identifier_fields`/`apply_to_aliases` here are byte-for-byte identical to `id_reassigner`'s. The only genuinely new logic is `id_for`'s base-name lookup. Three ~100-line stateful walkers means any fix to overflow handling, error wording, or traversal order has to land in three places. Not blocking this PR, but I'd like us to parameterize `ReassignFieldIds` with a strategy for "pick the id for an old id" (default fresh, overridable to check `base` first) and share the `apply_to_*` helpers instead of growing a third copy — worth at least a follow-up issue. ########## crates/iceberg/src/spec/schema/assign_fresh_ids.rs: ########## @@ -0,0 +1,407 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use super::utils::try_insert_field; +use super::*; + +pub(crate) fn assign_fresh_ids(schema: Schema, base: &Schema, start_from: i32) -> Result<Schema> { + let mut assigner = AssignFreshIds::new(&schema, base, start_from); + let Schema { + r#struct, + schema_id, + identifier_field_ids, + alias_to_id, + .. + } = schema; + let fields = assigner.assign_fields(r#struct.fields().to_vec())?; + let identifier_field_ids = assigner.apply_to_identifier_fields(identifier_field_ids)?; + let alias_to_id = assigner.apply_to_aliases(alias_to_id)?; + + Schema::builder() + .with_schema_id(schema_id) + .with_fields(fields) + .with_identifier_field_ids(identifier_field_ids) + .with_alias(alias_to_id) + .build() +} + +struct AssignFreshIds { + next_field_id: i32, + target_names: HashMap<i32, String>, + base_ids: HashMap<String, i32>, + old_to_new_id: HashMap<i32, i32>, +} + +impl AssignFreshIds { + fn new(target: &Schema, base: &Schema, start_from: i32) -> Self { + Self { + next_field_id: start_from, + target_names: target.field_id_to_name_map().clone(), Review Comment: `new` only borrows `target`, so this deep-clones the whole `HashMap<i32, String>` even though the caller drops `schema` immediately after. Since `assign_fresh_ids` already destructures `schema`, pull `id_to_name` out of the destructure and take it by value: ```rust let Schema { r#struct, schema_id, identifier_field_ids, alias_to_id, id_to_name, .. } = schema; let mut assigner = AssignFreshIds::new(id_to_name, base, start_from); ``` Saves a full copy of every field name on wide schemas. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
