Skip to content

fix(spec): allow void partition fields to reuse their source column name #3215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dchahla wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(spec): allow void partition fields to reuse their source column name #3215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c88afe7
fix(spec): allow void partition fields to reuse their source column name
Sep 13, 2026
f967d35
Merge branch 'main' into fix/void-partition-name-collision
dchahla Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 90 additions & 3 deletions crates/iceberg/src/spec/partition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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) {

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 `{}`",

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

field.name, schema_collision.id, field.source_id
),
))
Expand All @@ -618,7 +621,7 @@ impl PartitionSpecBuilder {
Err(Error::new(
ErrorKind::DataInvalid,
format!(
"Cannot create partition with name: '{}' that conflicts with schema field and is not an identity transform.",
"Cannot create partition with name: '{}' that conflicts with schema field and is not an identity or void transform.",
field.name
),
))
Expand Down Expand Up @@ -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();

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

#[test]
fn test_bind_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();

let spec = UnboundPartitionSpec::builder()
.with_spec_id(1)
.add_partition_field(1, "id", Transform::Void)
.unwrap()
.build()
.bind(schema.clone())
.unwrap();

assert_eq!(spec.fields()[0].name, "id");
assert_eq!(spec.fields()[0].transform, Transform::Void);

UnboundPartitionSpec::builder()
.with_spec_id(1)
.add_partition_field(2, "id", Transform::Void)
.unwrap()
.build()
.bind(schema)
.unwrap_err();
}

#[test]
fn test_builder_all_source_ids_must_exist() {
let schema = Schema::builder()
Expand Down
83 changes: 74 additions & 9 deletions crates/iceberg/src/spec/table_metadata_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,12 +765,13 @@ impl TableMetadataBuilder {
/// Validate partition field names against schema field names across all historical schemas.
///
/// Due to Iceberg's multi-version property, partition fields can share names with schema fields
/// if they meet specific requirements (identity transform + matching source field ID).
/// if they meet specific requirements (identity or void transform + matching source field ID).
/// This validation enforces those rules across all historical schema versions.
///
/// # Errors
/// - Partition field name conflicts with schema field name but doesn't use identity transform.
/// - Partition field uses identity transform but references wrong source field ID.
/// - Partition field name conflicts with schema field name but uses neither an identity nor a
/// void transform.
/// - Partition field uses an identity or void transform but references wrong source field ID.
fn validate_partition_field_names(&self, unbound_spec: &UnboundPartitionSpec) -> Result<()> {
if self.metadata.schemas.is_empty() {
return Ok(());
Expand All @@ -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!(

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

partition_field.transform,
crate::spec::Transform::Identity | crate::spec::Transform::Void
);
let has_matching_source_id = schema_field.id == partition_field.source_id;

if !is_identity_transform {
if !is_allowed_transform {
return Err(Error::new(
ErrorKind::DataInvalid,
format!(
"Cannot create partition with name '{}' that conflicts with schema field and is not an identity transform.",
"Cannot create partition with name '{}' that conflicts with schema field and is not an identity or void transform.",
partition_field.name
),
));
Expand All @@ -807,7 +812,7 @@ impl TableMetadataBuilder {
return Err(Error::new(
ErrorKind::DataInvalid,
format!(
"Cannot create identity partition sourced from different field in schema. \
"Cannot create partition sourced from different field in schema. \
Field name '{}' has id `{}` in schema but partition source id is `{}`",
partition_field.name, schema_field.id, partition_field.source_id
),
Expand Down Expand Up @@ -2955,7 +2960,7 @@ mod tests {
assert!(error_message.contains(
"Cannot create partition with name 'existing_field' that conflicts with schema field"
));
assert!(error_message.contains("and is not an identity transform"));
assert!(error_message.contains("and is not an identity or void transform"));
}

#[test]
Expand Down Expand Up @@ -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))

@laskoviymishka laskoviymishka Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.unwrap();

// The source must still match the column the field is named after.
builder()
.add_partition_spec(spec(2, Transform::Void))
.unwrap_err();

// Other transforms are still rejected on a name collision.
builder()
.add_partition_spec(spec(1, Transform::Bucket(4)))
.unwrap_err();
}

#[test]
fn test_partition_spec_evolution_allows_non_conflicting_names() {
let initial_schema = Schema::builder()
Expand Down
Loading