From de96300e37d5a45092ab7c8d64c6d4e4bb669067 Mon Sep 17 00:00:00 2001 From: Prashant Kumar Singh Date: Wed, 12 Nov 2025 16:25:10 +0000 Subject: [PATCH 1/3] REST: Fix serde of tasks with multiple deletes --- .../iceberg/rest/TableScanResponseParser.java | 2 +- .../java/org/apache/iceberg/TestBase.java | 8 +- .../TestPlanTableScanResponseParser.java | 78 +++++++++++++++++++ 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java index 67f71c418440..900f6b9adb01 100644 --- a/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java @@ -105,8 +105,8 @@ public static void serializeScanTasks( if (fileScanTasks != null) { gen.writeArrayFieldStart(FILE_SCAN_TASKS); - Set deleteFileReferences = Sets.newHashSet(); for (FileScanTask fileScanTask : fileScanTasks) { + Set deleteFileReferences = Sets.newHashSet(); if (deleteFiles != null) { for (DeleteFile taskDelete : fileScanTask.deletes()) { deleteFileReferences.add(deleteFilePathToIndex.get(taskDelete.path().toString())); diff --git a/core/src/test/java/org/apache/iceberg/TestBase.java b/core/src/test/java/org/apache/iceberg/TestBase.java index 2b91e212627d..0929c1bd37e6 100644 --- a/core/src/test/java/org/apache/iceberg/TestBase.java +++ b/core/src/test/java/org/apache/iceberg/TestBase.java @@ -111,7 +111,7 @@ public class TestBase { .withPartitionPath("data_bucket=0") .withRecordCount(1) .build(); - static final DataFile FILE_B = + public static final DataFile FILE_B = DataFiles.builder(SPEC) .withPath("/path/to/data-b.parquet") .withFileSizeInBytes(10) @@ -119,7 +119,7 @@ public class TestBase { .withRecordCount(1) .withSplitOffsets(ImmutableList.of(1L)) .build(); - static final DeleteFile FILE_B_DELETES = + public static final DeleteFile FILE_B_DELETES = FileMetadata.deleteFileBuilder(SPEC) .ofPositionDeletes() .withPath("/path/to/data-b-deletes.parquet") @@ -138,7 +138,7 @@ public class TestBase { .withContentOffset(4) .withContentSizeInBytes(6) .build(); - static final DataFile FILE_C = + public static final DataFile FILE_C = DataFiles.builder(SPEC) .withPath("/path/to/data-c.parquet") .withFileSizeInBytes(10) @@ -146,7 +146,7 @@ public class TestBase { .withRecordCount(1) .withSplitOffsets(ImmutableList.of(2L, 8L)) .build(); - static final DeleteFile FILE_C2_DELETES = + public static final DeleteFile FILE_C2_DELETES = FileMetadata.deleteFileBuilder(SPEC) .ofEqualityDeletes(1) .withPath("/path/to/data-c-deletes.parquet") 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 5ddedcacae1d..03afb7a568a9 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 @@ -20,6 +20,10 @@ import static org.apache.iceberg.TestBase.FILE_A; import static org.apache.iceberg.TestBase.FILE_A_DELETES; +import static org.apache.iceberg.TestBase.FILE_B; +import static org.apache.iceberg.TestBase.FILE_B_DELETES; +import static org.apache.iceberg.TestBase.FILE_C; +import static org.apache.iceberg.TestBase.FILE_C2_DELETES; import static org.apache.iceberg.TestBase.PARTITION_SPECS_BY_ID; import static org.apache.iceberg.TestBase.SCHEMA; import static org.apache.iceberg.TestBase.SPEC; @@ -265,6 +269,80 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedToJson); } + @Test + public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { + ResidualEvaluator residualEvaluator = + ResidualEvaluator.of(SPEC, Expressions.alwaysTrue(), true); + + // Create three tasks, each with its own distinct delete file + FileScanTask taskA = + new BaseFileScanTask( + FILE_A, + new DeleteFile[] {FILE_A_DELETES}, + SchemaParser.toJson(SCHEMA), + PartitionSpecParser.toJson(SPEC), + residualEvaluator); + + FileScanTask taskB = + new BaseFileScanTask( + FILE_B, + new DeleteFile[] {FILE_B_DELETES}, + SchemaParser.toJson(SCHEMA), + PartitionSpecParser.toJson(SPEC), + residualEvaluator); + + FileScanTask taskC = + new BaseFileScanTask( + FILE_C, + new DeleteFile[] {FILE_C2_DELETES}, + SchemaParser.toJson(SCHEMA), + PartitionSpecParser.toJson(SPEC), + residualEvaluator); + + PlanTableScanResponse response = + PlanTableScanResponse.builder() + .withPlanStatus(PlanStatus.COMPLETED) + .withFileScanTasks(List.of(taskA, taskB, taskC)) + .withDeleteFiles(List.of(FILE_A_DELETES, FILE_B_DELETES, FILE_C2_DELETES)) + .withSpecsById(PARTITION_SPECS_BY_ID) + .build(); + + String json = PlanTableScanResponseParser.toJson(response); + + // Verify each task only references its own delete file + // Task A should reference index 0 (fileADeletes) + // Task B should reference index 1 (fileBDeletes) + // Task C should reference index 2 (fileCDeletes) + assertThat(json).contains("\"delete-file-references\":[0]"); + assertThat(json).contains("\"delete-file-references\":[1]"); + assertThat(json).contains("\"delete-file-references\":[2]"); + + // Verify that no task has accumulated references (e.g., [0,1] or [0,1,2]) + assertThat(json).doesNotContain("\"delete-file-references\":[0,1]"); + assertThat(json).doesNotContain("\"delete-file-references\":[0,1,2]"); + + // Round-trip test: deserialize and verify each task has the correct delete files + PlanTableScanResponse deserialized = + PlanTableScanResponseParser.fromJson(json, PARTITION_SPECS_BY_ID, false); + + assertThat(deserialized.fileScanTasks()).hasSize(3); + + // Task A should only have FILE_A_DELETES + assertThat(deserialized.fileScanTasks().get(0).deletes()).hasSize(1); + assertThat(deserialized.fileScanTasks().get(0).deletes().get(0).path().toString()) + .isEqualTo(FILE_A_DELETES.location()); + + // Task B should only have FILE_B_DELETES + assertThat(deserialized.fileScanTasks().get(1).deletes()).hasSize(1); + assertThat(deserialized.fileScanTasks().get(1).deletes().get(0).path().toString()) + .isEqualTo(FILE_B_DELETES.location()); + + // Task C should only have FILE_C2_DELETES + assertThat(deserialized.fileScanTasks().get(2).deletes()).hasSize(1); + assertThat(deserialized.fileScanTasks().get(2).deletes().get(0).path().toString()) + .isEqualTo(FILE_C2_DELETES.location()); + } + @Test public void roundTripSerdeWithoutDeleteFiles() { ResidualEvaluator residualEvaluator = From 5f3ea49ed7291efed526cc81f2399c60addedfbe Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Wed, 12 Nov 2025 19:05:31 -0800 Subject: [PATCH 2/3] Address review feedback --- .../TestPlanTableScanResponseParser.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) 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 03afb7a568a9..a211251cfe0b 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 @@ -325,21 +325,22 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { PlanTableScanResponse deserialized = PlanTableScanResponseParser.fromJson(json, PARTITION_SPECS_BY_ID, false); - assertThat(deserialized.fileScanTasks()).hasSize(3); + List deserializedFSTasks = deserialized.fileScanTasks(); + assertThat(deserializedFSTasks).hasSize(3); // Task A should only have FILE_A_DELETES - assertThat(deserialized.fileScanTasks().get(0).deletes()).hasSize(1); - assertThat(deserialized.fileScanTasks().get(0).deletes().get(0).path().toString()) + assertThat(deserializedFSTasks.get(0).deletes()).hasSize(1); + assertThat(deserializedFSTasks.get(0).deletes().get(0).location()) .isEqualTo(FILE_A_DELETES.location()); // Task B should only have FILE_B_DELETES - assertThat(deserialized.fileScanTasks().get(1).deletes()).hasSize(1); - assertThat(deserialized.fileScanTasks().get(1).deletes().get(0).path().toString()) + assertThat(deserializedFSTasks.get(1).deletes()).hasSize(1); + assertThat(deserializedFSTasks.get(1).deletes().get(0).location()) .isEqualTo(FILE_B_DELETES.location()); // Task C should only have FILE_C2_DELETES - assertThat(deserialized.fileScanTasks().get(2).deletes()).hasSize(1); - assertThat(deserialized.fileScanTasks().get(2).deletes().get(0).path().toString()) + assertThat(deserializedFSTasks.get(2).deletes()).hasSize(1); + assertThat(deserializedFSTasks.get(2).deletes().get(0).location()) .isEqualTo(FILE_C2_DELETES.location()); } From 0a393ab36e30873dcf18d650d51cd78cc1d0ed1c Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Thu, 13 Nov 2025 00:10:09 -0800 Subject: [PATCH 3/3] Address review feedbacks --- .../iceberg/rest/TableScanResponseParser.java | 4 +- .../TestPlanTableScanResponseParser.java | 122 +++++++++++++----- 2 files changed, 89 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java index 900f6b9adb01..084e74651a0c 100644 --- a/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java @@ -96,7 +96,7 @@ public static void serializeScanTasks( gen.writeArrayFieldStart(DELETE_FILES); for (int i = 0; i < deleteFiles.size(); i++) { DeleteFile deleteFile = deleteFiles.get(i); - deleteFilePathToIndex.put(String.valueOf(deleteFile.path()), i); + deleteFilePathToIndex.put(deleteFile.location(), i); ContentFileParser.toJson(deleteFiles.get(i), specsById.get(deleteFile.specId()), gen); } @@ -109,7 +109,7 @@ public static void serializeScanTasks( Set deleteFileReferences = Sets.newHashSet(); if (deleteFiles != null) { for (DeleteFile taskDelete : fileScanTask.deletes()) { - deleteFileReferences.add(deleteFilePathToIndex.get(taskDelete.path().toString())); + deleteFileReferences.add(deleteFilePathToIndex.get(taskDelete.location())); } } 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 a211251cfe0b..3a8563ce58b3 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 @@ -307,41 +307,93 @@ public void multipleTasksWithDifferentDeleteFilesDontAccumulateReferences() { .withSpecsById(PARTITION_SPECS_BY_ID) .build(); - String json = PlanTableScanResponseParser.toJson(response); - - // Verify each task only references its own delete file - // Task A should reference index 0 (fileADeletes) - // Task B should reference index 1 (fileBDeletes) - // Task C should reference index 2 (fileCDeletes) - assertThat(json).contains("\"delete-file-references\":[0]"); - assertThat(json).contains("\"delete-file-references\":[1]"); - assertThat(json).contains("\"delete-file-references\":[2]"); - - // Verify that no task has accumulated references (e.g., [0,1] or [0,1,2]) - assertThat(json).doesNotContain("\"delete-file-references\":[0,1]"); - assertThat(json).doesNotContain("\"delete-file-references\":[0,1,2]"); - - // Round-trip test: deserialize and verify each task has the correct delete files - PlanTableScanResponse deserialized = - PlanTableScanResponseParser.fromJson(json, PARTITION_SPECS_BY_ID, false); - - List deserializedFSTasks = deserialized.fileScanTasks(); - assertThat(deserializedFSTasks).hasSize(3); - - // Task A should only have FILE_A_DELETES - assertThat(deserializedFSTasks.get(0).deletes()).hasSize(1); - assertThat(deserializedFSTasks.get(0).deletes().get(0).location()) - .isEqualTo(FILE_A_DELETES.location()); - - // Task B should only have FILE_B_DELETES - assertThat(deserializedFSTasks.get(1).deletes()).hasSize(1); - assertThat(deserializedFSTasks.get(1).deletes().get(0).location()) - .isEqualTo(FILE_B_DELETES.location()); - - // Task C should only have FILE_C2_DELETES - assertThat(deserializedFSTasks.get(2).deletes()).hasSize(1); - assertThat(deserializedFSTasks.get(2).deletes().get(0).location()) - .isEqualTo(FILE_C2_DELETES.location()); + String expectedJson = + "{\n" + + " \"plan-status\" : \"completed\",\n" + + " \"delete-files\" : [ {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"POSITION_DELETES\",\n" + + " \"file-path\" : \"/path/to/data-a-deletes.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 0\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1\n" + + " }, {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"POSITION_DELETES\",\n" + + " \"file-path\" : \"/path/to/data-b-deletes.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 1\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1\n" + + " }, {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"EQUALITY_DELETES\",\n" + + " \"file-path\" : \"/path/to/data-c-deletes.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 2\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1,\n" + + " \"equality-ids\" : [ 1 ],\n" + + " \"sort-order-id\" : 0\n" + + " } ],\n" + + " \"file-scan-tasks\" : [ {\n" + + " \"data-file\" : {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"DATA\",\n" + + " \"file-path\" : \"/path/to/data-a.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 0\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1,\n" + + " \"sort-order-id\" : 0\n" + + " },\n" + + " \"delete-file-references\" : [ 0 ],\n" + + " \"residual-filter\" : true\n" + + " }, {\n" + + " \"data-file\" : {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"DATA\",\n" + + " \"file-path\" : \"/path/to/data-b.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 1\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1,\n" + + " \"split-offsets\" : [ 1 ],\n" + + " \"sort-order-id\" : 0\n" + + " },\n" + + " \"delete-file-references\" : [ 1 ],\n" + + " \"residual-filter\" : true\n" + + " }, {\n" + + " \"data-file\" : {\n" + + " \"spec-id\" : 0,\n" + + " \"content\" : \"DATA\",\n" + + " \"file-path\" : \"/path/to/data-c.parquet\",\n" + + " \"file-format\" : \"PARQUET\",\n" + + " \"partition\" : {\n" + + " \"1000\" : 2\n" + + " },\n" + + " \"file-size-in-bytes\" : 10,\n" + + " \"record-count\" : 1,\n" + + " \"split-offsets\" : [ 2, 8 ],\n" + + " \"sort-order-id\" : 0\n" + + " },\n" + + " \"delete-file-references\" : [ 2 ],\n" + + " \"residual-filter\" : true\n" + + " } ]\n" + + "}"; + String json = PlanTableScanResponseParser.toJson(response, true); + assertThat(json).isEqualTo(expectedJson); } @Test