Skip to content

Core: Disallow creation of invalid PartitionSpec #12887

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

Merged
Fokko merged 2 commits into from
May 7, 2025
Merged

Core: Disallow creation of invalid PartitionSpec #12887

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
fb390f2
Core: Disallow creation of invalid PartitionSpec
devinrsmith Apr 22, 2025
44d3281
review response
devinrsmith Apr 24, 2025
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
11 changes: 11 additions & 0 deletions api/src/main/java/org/apache/iceberg/PartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.apache.iceberg.transforms.Transforms;
import org.apache.iceberg.transforms.UnknownTransform;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.types.Types.StructType;

Expand Down Expand Up @@ -624,6 +625,7 @@ PartitionSpec buildUnchecked() {
}

static void checkCompatibility(PartitionSpec spec, Schema schema) {
final Map<Integer, Integer> parents = TypeUtil.indexParents(schema.asStruct());
for (PartitionField field : spec.fields) {
Type sourceType = schema.findType(field.sourceId());
Transform<?, ?> transform = field.transform();
Expand All @@ -644,6 +646,15 @@ static void checkCompatibility(PartitionSpec spec, Schema schema) {
"Invalid source type %s for transform: %s",
sourceType,
transform);
// The only valid parent types for a PartitionField are StructTypes. This must be checked
// recursively.
Integer parentId = parents.get(field.sourceId());
while (parentId != null) {
Type parentType = schema.findType(parentId);
ValidationException.check(
parentType.isStructType(), "Invalid partition field parent: %s", parentType);
parentId = parents.get(parentId);
}
}
}
}
Expand Down
101 changes: 101 additions & 0 deletions api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,105 @@ private static Object[][] unsupportedFieldsProvider() {
{10, "unknown_partition1", "Invalid source type unknown for transform: bucket[5]"}
};
}

@Test
void testSourceIdNotFound() {
assertThatThrownBy(
() ->
PartitionSpec.builderFor(SCHEMA)
.add(99, 1000, "Test", Transforms.identity())
.build())
.isInstanceOf(ValidationException.class)
.hasMessage("Cannot find source column for partition field: 1000: Test: identity(99)");
}

@Test
void testPartitionFieldInStruct() {
final Schema schema =
new Schema(
NestedField.required(SCHEMA.highestFieldId() + 1, "MyStruct", SCHEMA.asStruct()));
PartitionSpec.builderFor(schema).identity("MyStruct.id").build();
}

@Test
void testPartitionFieldInStructInStruct() {
final Schema schema =
new Schema(
NestedField.optional(
SCHEMA.highestFieldId() + 2,
"Outer",
Types.StructType.of(
NestedField.required(
SCHEMA.highestFieldId() + 1, "Inner", SCHEMA.asStruct()))));
PartitionSpec.builderFor(schema).identity("Outer.Inner.id").build();
}

@Test
void testPartitionFieldInList() {
final Schema schema =
new Schema(
NestedField.required(
2, "MyList", Types.ListType.ofRequired(1, Types.IntegerType.get())));
assertThatThrownBy(() -> PartitionSpec.builderFor(schema).identity("MyList.element").build())
.isInstanceOf(ValidationException.class)
.hasMessage("Invalid partition field parent: list<int>");
}

@Test
void testPartitionFieldInStructInList() {
final Schema schema =
new Schema(
NestedField.required(
3,
"MyList",
Types.ListType.ofRequired(
2,
Types.StructType.of(NestedField.optional(1, "Foo", Types.IntegerType.get())))));
assertThatThrownBy(
() -> PartitionSpec.builderFor(schema).identity("MyList.element.Foo").build())
.isInstanceOf(ValidationException.class)
.hasMessage("Invalid partition field parent: list<struct<1: Foo: optional int>>");
}

@Test
void testPartitionFieldInMap() {
final Schema schema =
new Schema(
NestedField.required(
3,
"MyMap",
Types.MapType.ofRequired(1, 2, Types.IntegerType.get(), Types.IntegerType.get())));

assertThatThrownBy(() -> PartitionSpec.builderFor(schema).identity("MyMap.key").build())
.isInstanceOf(ValidationException.class)
.hasMessage("Invalid partition field parent: map<int, int>");

assertThatThrownBy(() -> PartitionSpec.builderFor(schema).identity("MyMap.value").build())
.isInstanceOf(ValidationException.class)
.hasMessage("Invalid partition field parent: map<int, int>");
}

@Test
void testPartitionFieldInStructInMap() {
final Schema schema =
new Schema(
NestedField.required(
5,
"MyMap",
Types.MapType.ofRequired(
3,
4,
Types.StructType.of(NestedField.optional(1, "Foo", Types.IntegerType.get())),
Types.StructType.of(NestedField.optional(2, "Bar", Types.IntegerType.get())))));

assertThatThrownBy(() -> PartitionSpec.builderFor(schema).identity("MyMap.key.Foo").build())
.isInstanceOf(ValidationException.class)
.hasMessage(
"Invalid partition field parent: map<struct<1: Foo: optional int>, struct<2: Bar: optional int>>");

assertThatThrownBy(() -> PartitionSpec.builderFor(schema).identity("MyMap.value.Bar").build())
.isInstanceOf(ValidationException.class)
.hasMessage(
"Invalid partition field parent: map<struct<1: Foo: optional int>, struct<2: Bar: optional int>>");
}
}