From 9f110729ccce5ae70f5270a156146ecc2d0e97c4 Mon Sep 17 00:00:00 2001 From: geruh Date: Wed, 26 Nov 2025 20:01:30 -0800 Subject: [PATCH 1/3] Core: Align ContentFile partition JSON with REST spec --- .../org/apache/iceberg/ContentFileParser.java | 53 ++++++++++++++----- .../apache/iceberg/TestContentFileParser.java | 47 +++++++++++----- .../apache/iceberg/TestDataTaskParser.java | 4 +- .../iceberg/TestFileScanTaskParser.java | 12 ++--- ...TestFetchPlanningResultResponseParser.java | 6 +-- .../TestFetchScanTasksResponseParser.java | 6 +-- .../TestPlanTableScanResponseParser.java | 40 +++++--------- 7 files changed, 101 insertions(+), 67 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/ContentFileParser.java b/core/src/main/java/org/apache/iceberg/ContentFileParser.java index b48334d8222d..4b4da0f6ba00 100644 --- a/core/src/main/java/org/apache/iceberg/ContentFileParser.java +++ b/core/src/main/java/org/apache/iceberg/ContentFileParser.java @@ -25,6 +25,7 @@ import java.util.List; import java.util.Map; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; public class ContentFileParser { @@ -89,7 +90,7 @@ public static void toJson(ContentFile contentFile, PartitionSpec spec, JsonGe if (contentFile.partition() != null) { generator.writeFieldName(PARTITION); - SingleValueParser.toJson(spec.partitionType(), contentFile.partition(), generator); + partitionToJson(spec.partitionType(), contentFile.partition(), generator); } generator.writeNumberField(FILE_SIZE, contentFile.fileSizeInBytes()); @@ -152,18 +153,7 @@ public static ContentFile fromJson(JsonNode jsonNode, Map javaClass = spec.partitionType().fields().get(pos).type().typeId().javaClass(); - partitionData.set(pos, structLike.get(pos, javaClass)); - } + partitionData = partitionFromJson(spec.partitionType(), jsonNode.get(PARTITION)); } long fileSizeInBytes = JsonUtil.getLong(FILE_SIZE, jsonNode); @@ -301,4 +291,41 @@ private static Metrics metricsFromJson(JsonNode jsonNode) { lowerBounds, upperBounds); } + + private static void partitionToJson( + Types.StructType partitionType, StructLike partitionData, JsonGenerator generator) + throws IOException { + generator.writeStartArray(); + List fields = partitionType.fields(); + for (int pos = 0; pos < fields.size(); ++pos) { + Types.NestedField field = fields.get(pos); + Object partitionValue = partitionData.get(pos, Object.class); + SingleValueParser.toJson(field.type(), partitionValue, generator); + } + generator.writeEndArray(); + } + + private static PartitionData partitionFromJson( + Types.StructType partitionType, JsonNode partitionNode) { + Preconditions.checkArgument( + partitionNode.isArray(), + "Invalid partition data for content file: non-array (%s)", + partitionNode); + + List fields = partitionType.fields(); + Preconditions.checkArgument( + partitionNode.size() == fields.size(), + "Invalid partition data size: expected = %s, actual = %s", + fields.size(), + partitionNode.size()); + + PartitionData partitionData = new PartitionData(partitionType); + for (int pos = 0; pos < fields.size(); ++pos) { + Types.NestedField field = fields.get(pos); + Object partitionValue = SingleValueParser.fromJson(field.type(), partitionNode.get(pos)); + partitionData.set(pos, partitionValue); + } + + return partitionData; + } } diff --git a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java index 3f463f722e9e..14bcc5aa19bc 100644 --- a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java +++ b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java @@ -72,7 +72,7 @@ public void testDataFile(PartitionSpec spec, DataFile dataFile, String expectedJ assertThat(jsonStr).isEqualTo(expectedJson); JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); ContentFile deserializedContentFile = - ContentFileParser.fromJson(jsonNode, Map.of(TestBase.SPEC.specId(), spec)); + ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec)); assertThat(deserializedContentFile).isInstanceOf(DataFile.class); assertContentFileEquals(dataFile, deserializedContentFile, spec); } @@ -85,11 +85,34 @@ public void testDeleteFile(PartitionSpec spec, DeleteFile deleteFile, String exp assertThat(jsonStr).isEqualTo(expectedJson); JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); ContentFile deserializedContentFile = - ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), TestBase.SPEC)); + ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec)); assertThat(deserializedContentFile).isInstanceOf(DeleteFile.class); assertContentFileEquals(deleteFile, deserializedContentFile, spec); } + @Test + public void testPartitionJsonValidation() throws Exception { + PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); + String baseJson = + "{\"spec-id\":%s,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":%s,\"file-size-in-bytes\":10," + + "\"record-count\":1}"; + + JsonNode nonArrayPartition = + JsonUtil.mapper().readTree(String.format(baseJson, spec.specId(), "{\"1000\":1}")); + assertThatThrownBy( + () -> ContentFileParser.fromJson(nonArrayPartition, Map.of(spec.specId(), spec))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageStartingWith("Invalid partition data for content file: non-array"); + + JsonNode wrongSizePartition = + JsonUtil.mapper().readTree(String.format(baseJson, spec.specId(), "[]")); + assertThatThrownBy( + () -> ContentFileParser.fromJson(wrongSizePartition, Map.of(spec.specId(), spec))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid partition data size"); + } + private static Stream provideSpecAndDataFile() { return Stream.of( Arguments.of( @@ -128,17 +151,17 @@ private static DataFile dataFileWithRequiredOnly(PartitionSpec spec) { private static String dataFileJsonWithRequiredOnly(PartitionSpec spec) { if (spec.isUnpartitioned()) { return "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{},\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}"; + + "\"partition\":[],\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}"; } else { return "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":1},\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}"; + + "\"partition\":[1],\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}"; } } private static String dataFileJsonWithAllOptional(PartitionSpec spec) { if (spec.isUnpartitioned()) { return "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-with-stats.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{},\"file-size-in-bytes\":350,\"record-count\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[],\"file-size-in-bytes\":350,\"record-count\":10," + "\"column-sizes\":{\"keys\":[3,4],\"values\":[100,200]}," + "\"value-counts\":{\"keys\":[3,4],\"values\":[90,180]}," + "\"null-value-counts\":{\"keys\":[3,4],\"values\":[10,20]}," @@ -149,7 +172,7 @@ private static String dataFileJsonWithAllOptional(PartitionSpec spec) { + "\"split-offsets\":[128,256],\"sort-order-id\":1}"; } else { return "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-with-stats.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":1},\"file-size-in-bytes\":350,\"record-count\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[1],\"file-size-in-bytes\":350,\"record-count\":10," + "\"column-sizes\":{\"keys\":[3,4],\"values\":[100,200]}," + "\"value-counts\":{\"keys\":[3,4],\"values\":[90,180]}," + "\"null-value-counts\":{\"keys\":[3,4],\"values\":[10,20]}," @@ -245,7 +268,7 @@ private static DeleteFile deleteFileWithDataRef(PartitionSpec spec) { private static String deleteFileWithDataRefJson() { return "{\"spec-id\":0,\"content\":\"POSITION_DELETES\",\"file-path\":\"/path/to/delete.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":4},\"file-size-in-bytes\":1234," + + "\"file-format\":\"PARQUET\",\"partition\":[4],\"file-size-in-bytes\":1234," + "\"record-count\":10,\"referenced-data-file\":\"/path/to/data/file.parquet\"}"; } @@ -271,7 +294,7 @@ private static DeleteFile dv(PartitionSpec spec) { private static String dvJson() { return "{\"spec-id\":0,\"content\":\"POSITION_DELETES\",\"file-path\":\"/path/to/delete.puffin\"," - + "\"file-format\":\"PUFFIN\",\"partition\":{\"1000\":4},\"file-size-in-bytes\":1234,\"record-count\":10," + + "\"file-format\":\"PUFFIN\",\"partition\":[4],\"file-size-in-bytes\":1234,\"record-count\":10," + "\"referenced-data-file\":\"/path/to/data/file.parquet\",\"content-offset\":4,\"content-size-in-bytes\":40}"; } @@ -344,17 +367,17 @@ private static DeleteFile deleteFileWithAllOptional(PartitionSpec spec) { private static String deleteFileJsonWithRequiredOnly(PartitionSpec spec) { if (spec.isUnpartitioned()) { return "{\"spec-id\":0,\"content\":\"POSITION_DELETES\",\"file-path\":\"/path/to/delete-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{},\"file-size-in-bytes\":1234,\"record-count\":9}"; + + "\"file-format\":\"PARQUET\",\"partition\":[],\"file-size-in-bytes\":1234,\"record-count\":9}"; } else { return "{\"spec-id\":0,\"content\":\"POSITION_DELETES\",\"file-path\":\"/path/to/delete-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":9},\"file-size-in-bytes\":1234,\"record-count\":9}"; + + "\"file-format\":\"PARQUET\",\"partition\":[9],\"file-size-in-bytes\":1234,\"record-count\":9}"; } } private static String deleteFileJsonWithAllOptional(PartitionSpec spec) { if (spec.isUnpartitioned()) { return "{\"spec-id\":0,\"content\":\"EQUALITY_DELETES\",\"file-path\":\"/path/to/delete-with-stats.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{},\"file-size-in-bytes\":1234,\"record-count\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[],\"file-size-in-bytes\":1234,\"record-count\":10," + "\"column-sizes\":{\"keys\":[3,4],\"values\":[100,200]}," + "\"value-counts\":{\"keys\":[3,4],\"values\":[90,180]}," + "\"null-value-counts\":{\"keys\":[3,4],\"values\":[10,20]}," @@ -365,7 +388,7 @@ private static String deleteFileJsonWithAllOptional(PartitionSpec spec) { + "\"split-offsets\":[128],\"equality-ids\":[3],\"sort-order-id\":1}"; } else { return "{\"spec-id\":0,\"content\":\"EQUALITY_DELETES\",\"file-path\":\"/path/to/delete-with-stats.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":9},\"file-size-in-bytes\":1234,\"record-count\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[9],\"file-size-in-bytes\":1234,\"record-count\":10," + "\"column-sizes\":{\"keys\":[3,4],\"values\":[100,200]}," + "\"value-counts\":{\"keys\":[3,4],\"values\":[90,180]}," + "\"null-value-counts\":{\"keys\":[3,4],\"values\":[10,20]}," diff --git a/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java b/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java index 8048b6d24e88..424190b3361e 100644 --- a/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java +++ b/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java @@ -146,7 +146,7 @@ public void missingFields() throws Exception { + "\"value\":\"string\",\"value-required\":true}}]}," + "\"metadata-file\":{\"spec-id\":0,\"content\":\"DATA\"," + "\"file-path\":\"/tmp/metadata2.json\"," - + "\"file-format\":\"METADATA\",\"partition\":{}," + + "\"file-format\":\"METADATA\",\"partition\":[]," + "\"file-size-in-bytes\":0,\"record-count\":2,\"sort-order-id\":0}" + "}"; JsonNode missingTableRowsNode = mapper.reader().readTree(missingTableRowsStr); @@ -248,7 +248,7 @@ private String snapshotsDataTaskJson() { + "\"value\":\"string\",\"value-required\":true}}]}," + "\"metadata-file\":{\"spec-id\":0,\"content\":\"DATA\"," + "\"file-path\":\"/tmp/metadata2.json\"," - + "\"file-format\":\"METADATA\",\"partition\":{}," + + "\"file-format\":\"METADATA\",\"partition\":[]," + "\"file-size-in-bytes\":0,\"record-count\":2,\"sort-order-id\":0}," + "\"rows\":[{\"1\":\"2009-02-13T23:31:30+00:00\",\"2\":1,\"4\":\"append\"," + "\"5\":\"file:/tmp/manifest1.avro\"," diff --git a/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java b/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java index c4a9fdf2340a..a3f5e3290161 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java +++ b/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java @@ -85,14 +85,14 @@ private String fileScanTaskJsonWithoutTaskType() { + "\"spec\":{\"spec-id\":0,\"fields\":[{\"name\":\"data_bucket\"," + "\"transform\":\"bucket[16]\",\"source-id\":4,\"field-id\":1000}]}," + "\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"start\":0,\"length\":10," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}," + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}," + "{\"spec-id\":0,\"content\":\"EQUALITY_DELETES\",\"file-path\":\"/path/to/data-a2-deletes.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0},\"file-size-in-bytes\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[0],\"file-size-in-bytes\":10," + "\"record-count\":1,\"equality-ids\":[1],\"sort-order-id\":0}]," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}"; } @@ -105,14 +105,14 @@ private String fileScanTaskJson() { + "\"spec\":{\"spec-id\":0,\"fields\":[{\"name\":\"data_bucket\"," + "\"transform\":\"bucket[16]\",\"source-id\":4,\"field-id\":1000}]}," + "\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"start\":0,\"length\":10," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}," + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}," + "{\"spec-id\":0,\"content\":\"EQUALITY_DELETES\",\"file-path\":\"/path/to/data-a2-deletes.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0},\"file-size-in-bytes\":10," + + "\"file-format\":\"PARQUET\",\"partition\":[0],\"file-size-in-bytes\":10," + "\"record-count\":1,\"equality-ids\":[1],\"sort-order-id\":0}]," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}"; } diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java index 9db9e29530f1..8b82e9794d5d 100644 --- a/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java +++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java @@ -157,7 +157,7 @@ public void roundTripSerdeWithInvalidPlanStatusSubmittedWithDeleteFilesNoFileSca "{\"status\":\"submitted\"," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]" + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]" + "}"; assertThatThrownBy( @@ -195,10 +195,10 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() throws JsonProcessin "{\"status\":\"completed\"," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]," + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]," + "\"file-scan-tasks\":[" + "{\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"delete-file-references\":[0]," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}]" diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchScanTasksResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchScanTasksResponseParser.java index 01ca8288fb40..62de75e3818b 100644 --- a/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchScanTasksResponseParser.java +++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestFetchScanTasksResponseParser.java @@ -97,7 +97,7 @@ public void roundTripSerdeWithDeleteFilesNoFileScanTasksPresent() { "{\"plan-tasks\":[\"task1\",\"task2\"]," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]" + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]" + "}"; assertThatThrownBy( @@ -131,10 +131,10 @@ public void roundTripSerdeWithFileScanTasks() { "{" + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]," + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]," + "\"file-scan-tasks\":[" + "{\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"delete-file-references\":[0]," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}]" diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java index c2867ca3a25f..a59bbbd16b45 100644 --- a/core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java +++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java @@ -212,7 +212,7 @@ public void roundTripSerdeWithInvalidPlanStatusSubmittedWithDeleteFilesNoFileSca + "\"plan-id\":\"somePlanId\"," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]" + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]" + "}"; assertThatThrownBy( @@ -246,10 +246,10 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { "{\"status\":\"completed\"," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," - + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}]," + + "\"partition\":[0],\"file-size-in-bytes\":10,\"record-count\":1}]," + "\"file-scan-tasks\":[" + "{\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"delete-file-references\":[0]," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}]" @@ -319,9 +319,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"POSITION_DELETES\",\n" + " \"file-path\" : \"/path/to/data-a-deletes.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 0\n" - + " },\n" + + " \"partition\" : [ 0 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1\n" + " }, {\n" @@ -329,9 +327,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"POSITION_DELETES\",\n" + " \"file-path\" : \"/path/to/data-b-deletes.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 1\n" - + " },\n" + + " \"partition\" : [ 1 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1\n" + " }, {\n" @@ -339,9 +335,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"EQUALITY_DELETES\",\n" + " \"file-path\" : \"/path/to/data-c-deletes.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 2\n" - + " },\n" + + " \"partition\" : [ 2 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1,\n" + " \"equality-ids\" : [ 1 ],\n" @@ -353,9 +347,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"DATA\",\n" + " \"file-path\" : \"/path/to/data-a.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 0\n" - + " },\n" + + " \"partition\" : [ 0 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1,\n" + " \"sort-order-id\" : 0\n" @@ -368,9 +360,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"DATA\",\n" + " \"file-path\" : \"/path/to/data-b.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 1\n" - + " },\n" + + " \"partition\" : [ 1 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1,\n" + " \"split-offsets\" : [ 1 ],\n" @@ -384,9 +374,7 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + " \"content\" : \"DATA\",\n" + " \"file-path\" : \"/path/to/data-c.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 2\n" - + " },\n" + + " \"partition\" : [ 2 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1,\n" + " \"split-offsets\" : [ 2, 8 ],\n" @@ -422,7 +410,7 @@ public void roundTripSerdeWithoutDeleteFiles() { "{\"status\":\"completed\"," + "\"file-scan-tasks\":[" + "{\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-format\":\"PARQUET\",\"partition\":[0]," + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}]" + "}"; @@ -623,9 +611,7 @@ public void roundTripSerdeWithValidStatusAndFileScanTasksAndCredentials() { + " \"content\" : \"POSITION_DELETES\",\n" + " \"file-path\" : \"/path/to/data-a-deletes.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 0\n" - + " },\n" + + " \"partition\" : [ 0 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1\n" + " } ],\n" @@ -635,9 +621,7 @@ public void roundTripSerdeWithValidStatusAndFileScanTasksAndCredentials() { + " \"content\" : \"DATA\",\n" + " \"file-path\" : \"/path/to/data-a.parquet\",\n" + " \"file-format\" : \"PARQUET\",\n" - + " \"partition\" : {\n" - + " \"1000\" : 0\n" - + " },\n" + + " \"partition\" : [ 0 ],\n" + " \"file-size-in-bytes\" : 10,\n" + " \"record-count\" : 1,\n" + " \"sort-order-id\" : 0\n" From f4c3f6bd2fa48b901b240fcb1cd7c1d54702fa67 Mon Sep 17 00:00:00 2001 From: geruh Date: Fri, 28 Nov 2025 14:50:15 -0800 Subject: [PATCH 2/3] Handle backwards compat --- .../org/apache/iceberg/ContentFileParser.java | 44 ++++++---- .../apache/iceberg/TestContentFileParser.java | 82 +++++++++++++++---- .../apache/iceberg/TestDataTaskParser.java | 17 ++++ .../iceberg/TestFileScanTaskParser.java | 32 ++++++++ 4 files changed, 146 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/ContentFileParser.java b/core/src/main/java/org/apache/iceberg/ContentFileParser.java index 4b4da0f6ba00..7fe48db0efa8 100644 --- a/core/src/main/java/org/apache/iceberg/ContentFileParser.java +++ b/core/src/main/java/org/apache/iceberg/ContentFileParser.java @@ -307,23 +307,37 @@ private static void partitionToJson( private static PartitionData partitionFromJson( Types.StructType partitionType, JsonNode partitionNode) { - Preconditions.checkArgument( - partitionNode.isArray(), - "Invalid partition data for content file: non-array (%s)", - partitionNode); - List fields = partitionType.fields(); - Preconditions.checkArgument( - partitionNode.size() == fields.size(), - "Invalid partition data size: expected = %s, actual = %s", - fields.size(), - partitionNode.size()); - PartitionData partitionData = new PartitionData(partitionType); - for (int pos = 0; pos < fields.size(); ++pos) { - Types.NestedField field = fields.get(pos); - Object partitionValue = SingleValueParser.fromJson(field.type(), partitionNode.get(pos)); - partitionData.set(pos, partitionValue); + + if (partitionNode.isArray()) { + Preconditions.checkArgument( + partitionNode.size() == fields.size(), + "Invalid partition data size: expected = %s, actual = %s", + fields.size(), + partitionNode.size()); + + for (int pos = 0; pos < fields.size(); ++pos) { + Types.NestedField field = fields.get(pos); + Object partitionValue = SingleValueParser.fromJson(field.type(), partitionNode.get(pos)); + partitionData.set(pos, partitionValue); + } + } else if (partitionNode.isObject()) { + // Handle legacy partition object serialization + for (int pos = 0; pos < fields.size(); ++pos) { + Types.NestedField field = fields.get(pos); + String fieldId = String.valueOf(field.fieldId()); + if (partitionNode.has(fieldId)) { + Object partitionValue = + SingleValueParser.fromJson(field.type(), partitionNode.get(fieldId)); + partitionData.set(pos, partitionValue); + } + } + } else { + throw new IllegalArgumentException( + String.format( + "Invalid partition data for content file: expected array or object (%s)", + partitionNode)); } return partitionData; diff --git a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java index 14bcc5aa19bc..e7fdfc2bc7d5 100644 --- a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java +++ b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java @@ -91,28 +91,82 @@ public void testDeleteFile(PartitionSpec spec, DeleteFile deleteFile, String exp } @Test - public void testPartitionJsonValidation() throws Exception { + public void testPartitionJsonArrayWrongSize() throws Exception { PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); - String baseJson = - "{\"spec-id\":%s,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," - + "\"file-format\":\"PARQUET\",\"partition\":%s,\"file-size-in-bytes\":10," + String jsonStr = + "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":[],\"file-size-in-bytes\":10," + "\"record-count\":1}"; - JsonNode nonArrayPartition = - JsonUtil.mapper().readTree(String.format(baseJson, spec.specId(), "{\"1000\":1}")); - assertThatThrownBy( - () -> ContentFileParser.fromJson(nonArrayPartition, Map.of(spec.specId(), spec))) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageStartingWith("Invalid partition data for content file: non-array"); + JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); - JsonNode wrongSizePartition = - JsonUtil.mapper().readTree(String.format(baseJson, spec.specId(), "[]")); - assertThatThrownBy( - () -> ContentFileParser.fromJson(wrongSizePartition, Map.of(spec.specId(), spec))) + assertThatThrownBy(() -> ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec))) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Invalid partition data size"); } + @Test + public void testPartitionJsonInvalidType() throws Exception { + PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); + String jsonStr = + "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":\"invalid\",\"file-size-in-bytes\":10," + + "\"record-count\":1}"; + + JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); + + assertThatThrownBy(() -> ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("expected array or object"); + } + + @Test + public void testParsesFieldIdPartitionMap() throws Exception { + PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); + String legacyJson = + "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":\"foo\"},\"file-size-in-bytes\":10," + + "\"record-count\":1}"; + + JsonNode jsonNode = JsonUtil.mapper().readTree(legacyJson); + ContentFile deserializedContentFile = + ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec)); + + assertThat(deserializedContentFile).isInstanceOf(DataFile.class); + assertThat(deserializedContentFile.partition().get(0, String.class)).isEqualTo("foo"); + } + + @Test + public void testPartitionArrayRespectsSpecOrder() throws Exception { + PartitionSpec spec = + PartitionSpec.builderFor(TestBase.SCHEMA).identity("id").identity("data").build(); + + PartitionData partitionData = new PartitionData(spec.partitionType()); + partitionData.set(0, 4); + partitionData.set(1, "foo"); + + DataFile dataFile = + DataFiles.builder(spec) + .withPath("/path/to/data.parquet") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withPartition(partitionData) + .build(); + + String jsonStr = ContentFileParser.toJson(dataFile, spec); + + // Verify partition values are serialized as array in correct order + assertThat(jsonStr).contains("\"partition\":[4,\"foo\"]"); + + JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); + ContentFile deserializedContentFile = + ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec)); + + assertThat(deserializedContentFile).isInstanceOf(DataFile.class); + assertThat(deserializedContentFile.partition().get(0, Integer.class)).isEqualTo(4); + assertThat(deserializedContentFile.partition().get(1, String.class)).isEqualTo("foo"); + } + private static Stream provideSpecAndDataFile() { return Stream.of( Arguments.of( diff --git a/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java b/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java index 424190b3361e..758c11db788a 100644 --- a/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java +++ b/core/src/test/java/org/apache/iceberg/TestDataTaskParser.java @@ -164,6 +164,23 @@ public void roundTripSerde() { assertDataTaskEquals(dataTask, deserializedTask); } + @Test + public void testDataTaskParsesFieldIdPartitionMap() { + String jsonStr = + "{\"task-type\":\"data-task\"," + + "\"schema\":{\"type\":\"struct\",\"schema-id\":0," + + "\"fields\":[{\"id\":1,\"name\":\"committed_at\",\"required\":true,\"type\":\"timestamptz\"}]}," + + "\"projection\":{\"type\":\"struct\",\"schema-id\":0," + + "\"fields\":[{\"id\":1,\"name\":\"committed_at\",\"required\":true,\"type\":\"timestamptz\"}]}," + + "\"metadata-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/tmp/metadata.json\"," + + "\"file-format\":\"METADATA\",\"partition\":{},\"file-size-in-bytes\":0,\"record-count\":1,\"sort-order-id\":0}," + + "\"rows\":[{\"1\":\"2009-02-13T23:31:30+00:00\"}]}"; + + StaticDataTask deserializedTask = (StaticDataTask) ScanTaskParser.fromJson(jsonStr, true); + + assertThat(deserializedTask.metadataFile().partition().size()).isEqualTo(0); + } + private DataTask createDataTask() { Map summary1 = ImmutableMap.of( diff --git a/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java b/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java index a3f5e3290161..882c2b33496d 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java +++ b/core/src/test/java/org/apache/iceberg/TestFileScanTaskParser.java @@ -62,6 +62,18 @@ public void testScanTaskParserWithoutTaskTypeField(boolean caseSensitive) { assertFileScanTaskEquals(fileScanTask, deserializedTask, spec, caseSensitive); } + @Test + public void testFileScanTaskParsesFieldIdPartitionMap() { + boolean caseSensitive = true; + PartitionSpec spec = TestBase.SPEC; + FileScanTask expected = createFileScanTask(spec, caseSensitive); + + FileScanTask deserializedTask = + ScanTaskParser.fromJson(fileScanTaskFieldIdPartitionMapJson(), caseSensitive); + + assertFileScanTaskEquals(expected, deserializedTask, spec, caseSensitive); + } + private FileScanTask createFileScanTask(PartitionSpec spec, boolean caseSensitive) { ResidualEvaluator residualEvaluator; if (spec.isUnpartitioned()) { @@ -117,6 +129,26 @@ private String fileScanTaskJson() { + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}"; } + private String fileScanTaskFieldIdPartitionMapJson() { + return "{\"task-type\":\"file-scan-task\"," + + "\"schema\":{\"type\":\"struct\",\"schema-id\":0,\"fields\":[" + + "{\"id\":3,\"name\":\"id\",\"required\":true,\"type\":\"int\"}," + + "{\"id\":4,\"name\":\"data\",\"required\":true,\"type\":\"string\"}]}," + + "\"spec\":{\"spec-id\":0,\"fields\":[{\"name\":\"data_bucket\"," + + "\"transform\":\"bucket[16]\",\"source-id\":4,\"field-id\":1000}]}," + + "\"data-file\":{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data-a.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0}," + + "\"file-size-in-bytes\":10,\"record-count\":1,\"sort-order-id\":0}," + + "\"start\":0,\"length\":10," + + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," + + "\"file-path\":\"/path/to/data-a-deletes.parquet\",\"file-format\":\"PARQUET\"," + + "\"partition\":{\"1000\":0},\"file-size-in-bytes\":10,\"record-count\":1}," + + "{\"spec-id\":0,\"content\":\"EQUALITY_DELETES\",\"file-path\":\"/path/to/data-a2-deletes.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":0},\"file-size-in-bytes\":10," + + "\"record-count\":1,\"equality-ids\":[1],\"sort-order-id\":0}]," + + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}"; + } + private static void assertFileScanTaskEquals( FileScanTask expected, FileScanTask actual, PartitionSpec spec, boolean caseSensitive) { TestContentFileParser.assertContentFileEquals(expected.file(), actual.file(), spec); From 8914c3a2dd62fb42f06cac84995473b9703f43d4 Mon Sep 17 00:00:00 2001 From: geruh Date: Sat, 29 Nov 2025 02:24:03 -0800 Subject: [PATCH 3/3] address comments --- .../org/apache/iceberg/ContentFileParser.java | 21 ++++++++------ .../apache/iceberg/TestContentFileParser.java | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/ContentFileParser.java b/core/src/main/java/org/apache/iceberg/ContentFileParser.java index 7fe48db0efa8..1f23d5227eb3 100644 --- a/core/src/main/java/org/apache/iceberg/ContentFileParser.java +++ b/core/src/main/java/org/apache/iceberg/ContentFileParser.java @@ -323,15 +323,18 @@ private static PartitionData partitionFromJson( partitionData.set(pos, partitionValue); } } else if (partitionNode.isObject()) { - // Handle legacy partition object serialization - for (int pos = 0; pos < fields.size(); ++pos) { - Types.NestedField field = fields.get(pos); - String fieldId = String.valueOf(field.fieldId()); - if (partitionNode.has(fieldId)) { - Object partitionValue = - SingleValueParser.fromJson(field.type(), partitionNode.get(fieldId)); - partitionData.set(pos, partitionValue); - } + // Handle partition struct object format, which serializes by field ID and skips + // null partition values + Preconditions.checkState( + partitionNode.size() <= fields.size(), + "Invalid partition data size: expected <= %s, actual = %s", + fields.size(), + partitionNode.size()); + + StructLike structLike = (StructLike) SingleValueParser.fromJson(partitionType, partitionNode); + for (int pos = 0; pos < partitionData.size(); ++pos) { + Class javaClass = fields.get(pos).type().typeId().javaClass(); + partitionData.set(pos, structLike.get(pos, javaClass)); } } else { throw new IllegalArgumentException( diff --git a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java index e7fdfc2bc7d5..60805c70c856 100644 --- a/core/src/test/java/org/apache/iceberg/TestContentFileParser.java +++ b/core/src/test/java/org/apache/iceberg/TestContentFileParser.java @@ -136,6 +136,34 @@ public void testParsesFieldIdPartitionMap() throws Exception { assertThat(deserializedContentFile.partition().get(0, String.class)).isEqualTo("foo"); } + @Test + public void testPartitionStructObjectContainsExtraField() throws Exception { + PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); + String jsonStr = + "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":{\"1000\":\"foo\",\"9999\":\"bar\"}," + + "\"file-size-in-bytes\":10,\"record-count\":1}"; + + JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); + assertThatThrownBy(() -> ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec))) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Invalid partition data size"); + } + + @Test + public void testPartitionStructObjectEmptyIsNull() throws Exception { + PartitionSpec spec = PartitionSpec.builderFor(TestBase.SCHEMA).identity("data").build(); + String jsonStr = + "{\"spec-id\":0,\"content\":\"DATA\",\"file-path\":\"/path/to/data.parquet\"," + + "\"file-format\":\"PARQUET\",\"partition\":{},\"file-size-in-bytes\":10," + + "\"record-count\":1}"; + + JsonNode jsonNode = JsonUtil.mapper().readTree(jsonStr); + ContentFile contentFile = ContentFileParser.fromJson(jsonNode, Map.of(spec.specId(), spec)); + assertThat(contentFile).isInstanceOf(DataFile.class); + assertThat(contentFile.partition().get(0, String.class)).isNull(); + } + @Test public void testPartitionArrayRespectsSpecOrder() throws Exception { PartitionSpec spec =