laskoviymishka commented on code in PR #3215:
URL: https://github.com/apache/iceberg-rust/pull/3215#discussion_r4008065328
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -3154,6 +3159,66 @@ mod tests {
assert!(error.message().contains("Cannot add schema field
'bucket_data' because it conflicts with existing partition field name"));
}
+ #[test]
+ fn
test_partition_spec_evolution_allows_void_reusing_its_source_column_name() {
+ let initial_schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "id",
Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(2, "region",
Type::Primitive(PrimitiveType::String)).into(),
+ ])
+ .build()
+ .unwrap();
+
+ // The partition field id is pinned so the v1 sequential-id rule is
not what decides
+ // these cases; the name-collision rule is what is under test.
+ let spec = |source_id: i32, transform: Transform| {
+ UnboundPartitionSpec::builder()
+ .with_spec_id(1)
+ .add_partition_fields(vec![UnboundPartitionField {
+ source_id,
+ field_id: Some(1000),
+ name: "id".to_string(),
+ transform,
+ }])
+ .unwrap()
+ .build()
+ };
+
+ let metadata = TableMetadataBuilder::new(
+ initial_schema,
+ spec(1, Transform::Identity),
+ SortOrder::unsorted_order(),
+ TEST_LOCATION.to_string(),
+ FormatVersion::V1,
+ HashMap::new(),
+ )
+ .unwrap()
+ .build()
+ .unwrap()
+ .metadata;
+
+ let builder = || {
+ metadata.clone().into_builder(Some(
+
"s3://bucket/test/location/metadata/metadata1.json".to_string(),
+ ))
+ };
+
+ // Rewriting the identity field to void is how a v1 table drops a
partition field.
+ builder()
+ .add_partition_spec(spec(1, Transform::Void))
Review Comment:
This proves `add_partition_spec` succeeds, but the `spec` closure pins
`field_id: Some(1000)` and we never go through `build()`.
For a real v1 drop that pin isn't a test convenience —
`reuse_partition_field_ids` keys on `(source_id, transform)`, so once the
transform flips to void the old id isn't reused, the builder assigns a fresh
one, and the sequential-id gate rejects the single-field spec. So "unblocks the
v1 drop partition field workflow" only holds if the caller carries the old
`field_id` forward.
I'd add a case that omits the pin to show the sequential-id error is what
fires, and call out the pinning requirement in the description. wdyt?
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -602,14 +602,17 @@ impl PartitionSpecBuilder {
) -> Result<()> {
match schema.field_by_name(field.name.as_str()) {
Some(schema_collision) => {
- if field.transform == Transform::Identity {
+ // A void transform always produces null, so like identity it
cannot carry a
Review Comment:
The "like identity it cannot carry a value that disagrees" framing is a
little off — identity does carry the source column's value, that's the whole
point; void just always produces null. The reason the allowance is safe is that
both keep the name/`source_id` pairing internally consistent (and Java treats
them identically in `checkAndAddPartitionName`). I'd reword so it doesn't imply
identity is value-free.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -1272,6 +1275,90 @@ mod tests {
.unwrap_err();
}
+ #[test]
+ fn test_builder_collision_is_ok_for_void_transforms() {
+ let schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "id",
Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(2, "region",
Type::Primitive(PrimitiveType::String)).into(),
+ ])
+ .build()
+ .unwrap();
+
+ // A void field may reuse the name of the column it is sourced from,
which is how a
+ // v1 table drops a partition field.
+ let spec = PartitionSpec::builder(schema.clone())
+ .with_spec_id(1)
+ .add_unbound_field(UnboundPartitionField {
+ source_id: 1,
+ field_id: None,
+ name: "id".to_string(),
+ transform: Transform::Void,
+ })
+ .unwrap()
+ .build()
+ .unwrap();
+
+ assert_eq!(spec.fields().len(), 1);
+ assert_eq!(spec.fields()[0].name, "id");
+ assert_eq!(spec.fields()[0].source_id, 1);
+ assert_eq!(spec.fields()[0].transform, Transform::Void);
+
+ // The allowance is not specific to one column.
+ PartitionSpec::builder(schema.clone())
+ .with_spec_id(1)
+ .add_unbound_field(UnboundPartitionField {
+ source_id: 2,
+ field_id: None,
+ name: "region".to_string(),
+ transform: Transform::Void,
+ })
+ .unwrap()
+ .build()
+ .unwrap();
+
+ // Not OK for different source id, same as identity.
+ PartitionSpec::builder(schema)
+ .with_spec_id(1)
+ .add_unbound_field(UnboundPartitionField {
+ source_id: 2,
+ field_id: None,
+ name: "id".to_string(),
+ transform: Transform::Void,
+ })
+ .unwrap_err();
Review Comment:
This `unwrap_err()` doesn't check why it failed, so it can't distinguish a
name-collision rejection from any other error on that path. Same for the bind
case below (`:1359`) and the two evolution cases in `table_metadata_builder.rs`
(`:3214`, `:3219`).
It happens to fail for the right reason today because `add_unbound_field`
reaches `check_name_does_not_collide_with_schema` first, but a reordering of
the checks would let these pass while silently no longer exercising the rule.
I'd assert on the message — `contains("sourced from different field")` for
the source-id mismatch and `contains("identity or void transform")` for the
wrong-transform case — matching the existing `test_collision_with_schema_name`
pattern.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -602,14 +602,17 @@ impl PartitionSpecBuilder {
) -> Result<()> {
match schema.field_by_name(field.name.as_str()) {
Some(schema_collision) => {
- if field.transform == Transform::Identity {
+ // A void transform always produces null, so like identity it
cannot carry a
+ // value that disagrees with the schema column it shares a
name with. Rewriting
+ // an identity field to void is how a v1 table drops a
partition field.
+ if matches!(field.transform, Transform::Identity |
Transform::Void) {
if schema_collision.id == field.source_id {
Ok(())
} else {
Err(Error::new(
ErrorKind::DataInvalid,
format!(
- "Cannot create identity partition sourced from
different field in schema. Field name '{}' has id `{}` in schema but partition
source id is `{}`",
+ "Cannot create partition sourced from
different field in schema. Field name '{}' has id `{}` in schema but partition
source id is `{}`",
Review Comment:
Dropping "identity" here makes the message generic — it no longer tells the
user which transforms trigger the check, and the sibling message on the
wrong-transform branch does name "identity or void transform." I'd keep the
qualifier: "Cannot create identity or void partition sourced from different
field...".
While we're aligning strings, the two "conflicts with schema field" messages
also differ by a stray colon (`partition.rs:624` has `name: '{}'`,
`table_metadata_builder.rs:805` has `name '{}'`) — worth unifying since both
are public-facing.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -789,15 +790,19 @@ impl TableMetadataBuilder {
// If name exists in schemas, validate against current schema rules
if let Some(schema_field) =
current_schema.field_by_name(&partition_field.name) {
- let is_identity_transform =
- partition_field.transform ==
crate::spec::Transform::Identity;
+ // A void transform always produces null, so like identity it
cannot carry a
+ // value that disagrees with the schema column it shares a
name with.
+ let is_allowed_transform = matches!(
Review Comment:
This "allowed transforms for name-sharing" predicate now lives in two files
(here and `check_name_does_not_collide_with_schema` in `partition.rs`) with no
compile-time link, so a future third transform has to be added in both by hand.
A one-line cross-ref comment ("keep in sync with partition.rs") would cost
nothing.
Minor while we're here: `partition.rs` uses the imported `Transform` short
name — adding `Transform` to the `use` block here would let this read
`matches!(..., Transform::Identity | Transform::Void)` and match the other site.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -602,14 +602,17 @@ impl PartitionSpecBuilder {
) -> Result<()> {
match schema.field_by_name(field.name.as_str()) {
Some(schema_collision) => {
- if field.transform == Transform::Identity {
+ // A void transform always produces null, so like identity it
cannot carry a
+ // value that disagrees with the schema column it shares a
name with. Rewriting
+ // an identity field to void is how a v1 table drops a
partition field.
+ if matches!(field.transform, Transform::Identity |
Transform::Void) {
Review Comment:
The function doc comment just above (rule 2) still reads "AND the
transformation is identity" — it doesn't mention void, so it now contradicts
this branch. I'd update rule 2 to "identity or void" so the docstring matches
the code.
--
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]
