mbutrovich commented on code in PR #3412:
URL: https://github.com/apache/iceberg-rust/pull/3412#discussion_r4251783545
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -3107,6 +3111,65 @@ mod tests {
assert!(result.is_ok());
}
+ #[test]
+ fn
test_schema_evolution_allows_a_field_named_after_a_superseded_partition_field()
{
+ let initial_schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "data",
Type::Primitive(PrimitiveType::String)).into(),
+ ])
+ .build()
+ .unwrap();
+ let partition_spec = UnboundPartitionSpec::builder()
+ .with_spec_id(0)
+ .add_partition_field(
+ UnboundPartitionField::builder()
+ .source_ids(vec![1])
+ .name("bucket_data")
+ .transform(Transform::Bucket(16))
+ .build()
+ .unwrap(),
+ )
+ .unwrap()
+ .build();
+
+ let metadata = TableMetadataBuilder::new(
+ initial_schema,
+ partition_spec,
+ SortOrder::unsorted_order(),
+ TEST_LOCATION.to_string(),
+ FormatVersion::V2,
+ HashMap::new(),
+ )
+ .unwrap()
+ .build()
+ .unwrap()
+ .metadata;
+
+ // Stop partitioning by `bucket_data`. Spec 0 is retained, but no
longer current.
+ let metadata = metadata
+ .into_builder(Some("test_location".to_string()))
+
.add_default_partition_spec(UnboundPartitionSpec::builder().build())
+ .unwrap()
+ .build()
+ .unwrap()
+ .metadata;
+ assert_ne!(metadata.default_partition_spec().spec_id(), 0);
+ assert!(metadata.partition_specs.contains_key(&0));
+
+ let schema_with_the_name = Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "data",
Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(2, "bucket_data",
Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap();
+
+ metadata
+ .into_builder(Some("test_location".to_string()))
+ .add_current_schema(schema_with_the_name)
+ .unwrap();
Review Comment:
The new schema keeps `data` (id 1), which is the source column of
`bucket_data`. That is the case Java 1.11.0 rejects (see the comment on
`validate_schema_field_names`). Could the tests cover both sides: a schema that
drops `data` and adds `bucket_data`, which Java accepts, and one that keeps
`data`, which it rejects? A case that adds the schema and the new spec in the
same builder, in both orders, would cover the ordering question too.
Could this test also call `build()` and compare the resulting current schema
with the expected `Schema`? As written it checks only that `add_current_schema`
returns `Ok`, and `build()` is where the default spec is rebound to the new
current schema.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -695,27 +695,27 @@ impl TableMetadataBuilder {
.set_current_schema(Self::LAST_ADDED)
}
- /// Validate schema field names against partition field names across all
historical schemas.
+ /// Validate schema field names against the current partition spec's field
names.
///
- /// Due to Iceberg's multi-version property, this check ignores existing
schema fields
- /// that match partition names (schema evolution allows re-adding
previously removed fields).
- /// Only NEW field names that conflict with partition names are rejected.
+ /// A name that any schema already has is allowed, since Iceberg keeps
every schema and
+ /// evolution may re-add a dropped column. Superseded specs are not
consulted: they are kept
+ /// only to interpret old metadata, so the names they use stay available.
///
/// # Errors
- /// - Schema field name conflicts with partition field name but doesn't
exist in any historical schema.
+ /// - A field name new to the table conflicts with a current partition
field name.
fn validate_schema_field_names(&self, schema: &Schema) -> Result<()> {
if self.metadata.schemas.is_empty() {
return Ok(());
}
for field_name in schema.field_id_to_name_map().values() {
- let has_partition_conflict =
self.metadata.partition_name_exists(field_name);
+ let has_partition_conflict =
self.metadata.default_partition_name_exists(field_name);
Review Comment:
> So we reject schema changes Java accepts.
When I run the repro from #3411 against Iceberg Java 1.11.0
(`TableMetadata.buildFrom(unpartitioned).setCurrentSchema(schemaWithXBucket,
2).build()`), it fails with `IllegalArgumentException: Cannot create identity
partition sourced from different field in schema: x_bucket`.
As I read it, `setCurrentSchema` rebuilds every spec, not only the default
one ([`TableMetadata.java`
L1105-L1107](https://github.com/apache/iceberg/blob/843d36de699bbbacd1fc317e4ec3d824b600eccd/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1105-L1107)).
`updateSpecSchema` re-adds each field through `Builder.add(sourceId, fieldId,
name, transform)`
([L760-L772](https://github.com/apache/iceberg/blob/843d36de699bbbacd1fc317e4ec3d824b600eccd/core/src/main/java/org/apache/iceberg/TableMetadata.java#L760-L772)),
and `add` runs `checkAndAddPartitionName(name, sourceId)`
([`PartitionSpec.java`
L407-L427](https://github.com/apache/iceberg/blob/843d36de699bbbacd1fc317e4ec3d824b600eccd/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L407-L427)).
`buildUnchecked()` skips the compatibility check in `build`, but the name
check has already run in `add`. That check is skipped only when the field's
source column is no longer in the schema, which apache/iceberg#15967 added for
this case.
So Java accepts the name when the superseded partition field's source column
was dropped, and rejects it when the source column is still in the schema. The
[partitioning section of the
spec](https://github.com/apache/iceberg/blob/843d36de699bbbacd1fc317e4ec3d824b600eccd/format/spec.md?plain=1#L548-L567)
doesn't say anything about partition names colliding with schema names, so
Java's behavior is what we have to go on. What do you think about matching that
rule here? That would mean checking the fields of every spec, as `main` does,
and skipping a field whose source id isn't in the new schema.
That rule would also make the result independent of call order. With this
PR, the check reads the default spec at the time `add_schema` runs. Adding the
new schema and then the unpartitioned spec in one builder
(`add_current_schema(...)` then `add_default_partition_spec(...)`) fails, and
the reverse order succeeds. Java's `buildReplacement` uses the schema-first
order
([L741-L749](https://github.com/apache/iceberg/blob/843d36de699bbbacd1fc317e4ec3d824b600eccd/core/src/main/java/org/apache/iceberg/TableMetadata.java#L741-L749)).
For #3056, `buildReplacement` in Java 1.11.0 also rejects the #3411 repro
(keep `x`, add `x_bucket`, replace with an unpartitioned spec) with the same
error.
--
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]
