Address second round review feedback

sfc-gh-prsingh · sfc-gh-prsingh · commit c6f02acbb3af · 2025-08-12T17:39:31.000-07:00
diff --git a/core/src/main/java/org/apache/iceberg/TableScanResponseParser.java b/core/src/main/java/org/apache/iceberg/TableScanResponseParser.java
@@ -52,7 +52,7 @@ public static List<DeleteFile> parseDeleteFiles(
       return deleteFilesBuilder.build();
     }
 
-    return null;
+    return Lists.newArrayList();
   }
 
   public static List<FileScanTask> parseFileScanTasks(
@@ -85,7 +85,7 @@ public static void serializeScanTasks(
       JsonGenerator gen)
       throws IOException {
     Map<String, Integer> deleteFilePathToIndex = Maps.newHashMap();
-    if (deleteFiles != null) {
+    if (deleteFiles != null && !deleteFiles.isEmpty()) {
       Preconditions.checkArgument(
           specsById != null, "Cannot serialize response without specs by ID defined");
       gen.writeArrayFieldStart(DELETE_FILES);
diff --git a/core/src/main/java/org/apache/iceberg/rest/PlanStatus.java b/core/src/main/java/org/apache/iceberg/rest/PlanStatus.java
@@ -40,7 +40,7 @@ public String status() {
   public static PlanStatus fromName(String status) {
     Preconditions.checkArgument(status != null, "Status is null");
     try {
-      return PlanStatus.valueOf(status.toUpperCase(Locale.ENGLISH));
+      return PlanStatus.valueOf(status.toUpperCase(Locale.ROOT));
     } catch (IllegalArgumentException e) {
       throw new IllegalArgumentException(String.format("Invalid status name: %s", status), e);
     }
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java b/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
@@ -545,13 +545,8 @@ static class PlanTableScanResponseDeserializer<T extends PlanTableScanResponse>
     @Override
     public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
       JsonNode jsonNode = p.getCodec().readTree(p);
-      // Retrieve injectable values
-      @SuppressWarnings("unchecked")
-      Map<Integer, PartitionSpec> specsById =
-          (Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
 
-      boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
-      return (T) PlanTableScanResponseParser.fromJson(jsonNode, specsById, caseSensitive);
+      return (T) PlanTableScanResponseParser.fromJson(jsonNode, parseScanResponseContext(context));
     }
   }
 
@@ -569,13 +564,9 @@ static class FetchPlanningResultResponseDeserializer<T extends FetchPlanningResu
     @Override
     public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
       JsonNode jsonNode = p.getCodec().readTree(p);
-      // Retrieve injectable values
-      @SuppressWarnings("unchecked")
-      Map<Integer, PartitionSpec> specsById =
-          (Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
 
-      boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
-      return (T) FetchPlanningResultResponseParser.fromJson(jsonNode, specsById, caseSensitive);
+      return (T)
+          FetchPlanningResultResponseParser.fromJson(jsonNode, parseScanResponseContext(context));
     }
   }
 
@@ -593,13 +584,17 @@ static class FetchScanTaskResponseDeserializer<T extends FetchScanTasksResponse>
     @Override
     public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
       JsonNode jsonNode = p.getCodec().readTree(p);
-      // Retrieve injectable values
-      @SuppressWarnings("unchecked")
-      Map<Integer, PartitionSpec> specsById =
-          (Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
 
-      boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
-      return (T) FetchScanTasksResponseParser.fromJson(jsonNode, specsById, caseSensitive);
+      return (T) FetchScanTasksResponseParser.fromJson(jsonNode, parseScanResponseContext(context));
     }
   }
+
+  private static TableScanResponseContext parseScanResponseContext(DeserializationContext context)
+      throws IOException {
+    @SuppressWarnings("unchecked")
+    Map<Integer, PartitionSpec> specsById =
+        (Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
+    boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
+    return new TableScanResponseContext(specsById, caseSensitive);
+  }
 }
diff --git a/core/src/main/java/org/apache/iceberg/rest/TableScanResponseContext.java b/core/src/main/java/org/apache/iceberg/rest/TableScanResponseContext.java
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.util.Map;
+import org.apache.iceberg.PartitionSpec;
+
+public class TableScanResponseContext {
+  private final Map<Integer, PartitionSpec> specsById;
+  private final boolean caseSensitive;
+
+  public TableScanResponseContext(Map<Integer, PartitionSpec> specs, boolean caseSen) {
+    this.specsById = specs;
+    this.caseSensitive = caseSen;
+  }
+
+  public Map<Integer, PartitionSpec> getSpecsById() {
+    return specsById;
+  }
+
+  public boolean isCaseSensitive() {
+    return caseSensitive;
+  }
+}
diff --git a/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java b/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java
@@ -83,6 +83,7 @@ private PlanTableScanRequest(
     this.startSnapshotId = startSnapshotId;
     this.endSnapshotId = endSnapshotId;
     this.statsFields = statsFields;
+    validate();
   }
 
   @Override
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java
@@ -25,14 +25,14 @@
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.rest.RESTResponse;
 
-public abstract class BaseScanResponse implements RESTResponse {
+public abstract class BaseScanTaskResponse implements RESTResponse {
 
   private final List<String> planTasks;
   private final List<FileScanTask> fileScanTasks;
   private final List<DeleteFile> deleteFiles;
   private final Map<Integer, PartitionSpec> specsById;
 
-  protected BaseScanResponse(
+  protected BaseScanTaskResponse(
       List<String> planTasks,
       List<FileScanTask> fileScanTasks,
       List<DeleteFile> deleteFiles,
@@ -59,7 +59,7 @@ public Map<Integer, PartitionSpec> specsById() {
     return specsById;
   }
 
-  public abstract static class Builder<B extends Builder<B, R>, R extends BaseScanResponse> {
+  public abstract static class Builder<B extends Builder<B, R>, R extends BaseScanTaskResponse> {
     private List<String> planTasks;
     private List<FileScanTask> fileScanTasks;
     private List<DeleteFile> deleteFiles;
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java
@@ -26,7 +26,7 @@
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.rest.PlanStatus;
 
-public class FetchPlanningResultResponse extends BaseScanResponse {
+public class FetchPlanningResultResponse extends BaseScanTaskResponse {
   private final PlanStatus planStatus;
 
   private FetchPlanningResultResponse(
@@ -62,7 +62,7 @@ public void validate() {
   }
 
   public static class Builder
-      extends BaseScanResponse.Builder<Builder, FetchPlanningResultResponse> {
+      extends BaseScanTaskResponse.Builder<Builder, FetchPlanningResultResponse> {
     private Builder() {}
 
     private PlanStatus planStatus;
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponseParser.java
@@ -27,8 +27,10 @@
 import org.apache.iceberg.FileScanTask;
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.TableScanResponseParser;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.rest.PlanStatus;
+import org.apache.iceberg.rest.TableScanResponseContext;
 import org.apache.iceberg.util.JsonUtil;
 
 public class FetchPlanningResultResponseParser {
@@ -63,26 +65,30 @@ public static void toJson(FetchPlanningResultResponse response, JsonGenerator ge
     gen.writeEndObject();
   }
 
-  public static FetchPlanningResultResponse fromJson(
+  @VisibleForTesting
+  static FetchPlanningResultResponse fromJson(
       String json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
-    Preconditions.checkArgument(json != null, "Invalid fetchPlanningResult response: null");
+    Preconditions.checkArgument(
+        json != null, "Invalid fetchPlanningResult response: null or empty");
     return JsonUtil.parse(
         json,
         node -> {
-          return fromJson(node, specsById, caseSensitive);
+          return fromJson(node, new TableScanResponseContext(specsById, caseSensitive));
         });
   }
 
   public static FetchPlanningResultResponse fromJson(
-      JsonNode json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
+      JsonNode json, TableScanResponseContext context) {
     Preconditions.checkArgument(
         json != null && !json.isEmpty(), "Invalid fetchPlanningResult response: null or empty");
 
     PlanStatus planStatus = PlanStatus.fromName(JsonUtil.getString(PLAN_STATUS, json));
     List<String> planTasks = JsonUtil.getStringListOrNull(PLAN_TASKS, json);
-    List<DeleteFile> deleteFiles = TableScanResponseParser.parseDeleteFiles(json, specsById);
+    List<DeleteFile> deleteFiles =
+        TableScanResponseParser.parseDeleteFiles(json, context.getSpecsById());
     List<FileScanTask> fileScanTasks =
-        TableScanResponseParser.parseFileScanTasks(json, deleteFiles, specsById, caseSensitive);
+        TableScanResponseParser.parseFileScanTasks(
+            json, deleteFiles, context.getSpecsById(), context.isCaseSensitive());
     return FetchPlanningResultResponse.builder()
         .withPlanStatus(planStatus)
         .withPlanTasks(planTasks)
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponse.java
@@ -25,7 +25,7 @@
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 
-public class FetchScanTasksResponse extends BaseScanResponse {
+public class FetchScanTasksResponse extends BaseScanTaskResponse {
 
   private FetchScanTasksResponse(
       List<String> planTasks,
@@ -53,7 +53,8 @@ public static Builder builder() {
     return new Builder();
   }
 
-  public static class Builder extends BaseScanResponse.Builder<Builder, FetchScanTasksResponse> {
+  public static class Builder
+      extends BaseScanTaskResponse.Builder<Builder, FetchScanTasksResponse> {
     private Builder() {}
 
     @Override
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponseParser.java
@@ -27,7 +27,9 @@
 import org.apache.iceberg.FileScanTask;
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.TableScanResponseParser;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.rest.TableScanResponseContext;
 import org.apache.iceberg.util.JsonUtil;
 
 public class FetchScanTasksResponseParser {
@@ -59,24 +61,26 @@ public static void toJson(FetchScanTasksResponse response, JsonGenerator gen) th
     gen.writeEndObject();
   }
 
-  public static FetchScanTasksResponse fromJson(
+  @VisibleForTesting
+  static FetchScanTasksResponse fromJson(
       String json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
     Preconditions.checkArgument(json != null, "Cannot parse fetchScanTasks response from null");
     return JsonUtil.parse(
         json,
         node -> {
-          return fromJson(node, specsById, caseSensitive);
+          return fromJson(node, new TableScanResponseContext(specsById, caseSensitive));
         });
   }
 
-  public static FetchScanTasksResponse fromJson(
-      JsonNode json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
+  public static FetchScanTasksResponse fromJson(JsonNode json, TableScanResponseContext context) {
     Preconditions.checkArgument(
         json != null && !json.isEmpty(), "Invalid response: fetchScanTasksResponse null");
     List<String> planTasks = JsonUtil.getStringListOrNull(PLAN_TASKS, json);
-    List<DeleteFile> deleteFiles = TableScanResponseParser.parseDeleteFiles(json, specsById);
+    List<DeleteFile> deleteFiles =
+        TableScanResponseParser.parseDeleteFiles(json, context.getSpecsById());
     List<FileScanTask> fileScanTasks =
-        TableScanResponseParser.parseFileScanTasks(json, deleteFiles, specsById, caseSensitive);
+        TableScanResponseParser.parseFileScanTasks(
+            json, deleteFiles, context.getSpecsById(), context.isCaseSensitive());
     return FetchScanTasksResponse.builder()
         .withPlanTasks(planTasks)
         .withFileScanTasks(fileScanTasks)
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java
@@ -27,7 +27,7 @@
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.rest.PlanStatus;
 
-public class PlanTableScanResponse extends BaseScanResponse {
+public class PlanTableScanResponse extends BaseScanTaskResponse {
   private final PlanStatus planStatus;
   private final String planId;
 
@@ -72,10 +72,10 @@ public void validate() {
         "Invalid response: 'cancelled' is not a valid status for planTableScan");
     Preconditions.checkArgument(
         planStatus() == PlanStatus.COMPLETED || (planTasks() == null && fileScanTasks() == null),
-        "Invalid response: tasks can only be returned in a 'completed' status");
+        "Invalid response: tasks can only be defined when status is 'completed'");
     Preconditions.checkArgument(
         planStatus() == PlanStatus.SUBMITTED || planId() == null,
-        "Invalid response: plan id can only be returned in a 'submitted' status");
+        "Invalid response: plan id can only be defined when status is 'submitted'");
     if (fileScanTasks() == null || fileScanTasks().isEmpty()) {
       Preconditions.checkArgument(
           (deleteFiles() == null || deleteFiles().isEmpty()),
@@ -87,7 +87,7 @@ public static Builder builder() {
     return new Builder();
   }
 
-  public static class Builder extends BaseScanResponse.Builder<Builder, PlanTableScanResponse> {
+  public static class Builder extends BaseScanTaskResponse.Builder<Builder, PlanTableScanResponse> {
     private PlanStatus planStatus;
     private String planId;
 
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java
@@ -23,12 +23,14 @@
 import java.io.IOException;
 import java.util.List;
 import java.util.Map;
+import org.apache.hadoop.classification.VisibleForTesting;
 import org.apache.iceberg.DeleteFile;
 import org.apache.iceberg.FileScanTask;
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.TableScanResponseParser;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.rest.PlanStatus;
+import org.apache.iceberg.rest.TableScanResponseContext;
 import org.apache.iceberg.util.JsonUtil;
 
 public class PlanTableScanResponseParser {
@@ -69,29 +71,32 @@ public static void toJson(PlanTableScanResponse response, JsonGenerator gen) thr
     gen.writeEndObject();
   }
 
-  public static PlanTableScanResponse fromJson(
+  @VisibleForTesting
+  static PlanTableScanResponse fromJson(
       String json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
     Preconditions.checkArgument(
         json != null, "Cannot parse planTableScan response from empty or null object");
     return JsonUtil.parse(
         json,
         node -> {
-          return PlanTableScanResponseParser.fromJson(node, specsById, caseSensitive);
+          return PlanTableScanResponseParser.fromJson(
+              node, new TableScanResponseContext(specsById, caseSensitive));
         });
   }
 
-  public static PlanTableScanResponse fromJson(
-      JsonNode json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
+  public static PlanTableScanResponse fromJson(JsonNode json, TableScanResponseContext context) {
     Preconditions.checkArgument(
         json != null && !json.isEmpty(),
         "Cannot parse planTableScan response from empty or null object");
 
     PlanStatus planStatus = PlanStatus.fromName(JsonUtil.getString(PLAN_STATUS, json));
     String planId = JsonUtil.getStringOrNull(PLAN_ID, json);
     List<String> planTasks = JsonUtil.getStringListOrNull(PLAN_TASKS, json);
-    List<DeleteFile> deleteFiles = TableScanResponseParser.parseDeleteFiles(json, specsById);
+    List<DeleteFile> deleteFiles =
+        TableScanResponseParser.parseDeleteFiles(json, context.getSpecsById());
     List<FileScanTask> fileScanTasks =
-        TableScanResponseParser.parseFileScanTasks(json, deleteFiles, specsById, caseSensitive);
+        TableScanResponseParser.parseFileScanTasks(
+            json, deleteFiles, context.getSpecsById(), context.isCaseSensitive());
 
     return PlanTableScanResponse.builder()
         .withPlanId(planId)
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
@@ -30,7 +30,6 @@
 import com.fasterxml.jackson.core.JsonFactoryBuilder;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.InjectableValues;
-import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.ObjectReader;
 import java.util.List;
@@ -69,7 +68,7 @@ public void nullAndEmptyCheck() {
     assertThatThrownBy(
             () ->
                 FetchPlanningResultResponseParser.fromJson(
-                    (JsonNode) null, PARTITION_SPECS_BY_ID, false))
+                    (String) null, PARTITION_SPECS_BY_ID, false))
         .isInstanceOf(IllegalArgumentException.class)
         .hasMessage("Invalid fetchPlanningResult response: null or empty");
   }
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
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

Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ public String status() {`
`40`	`40`	`public static PlanStatus fromName(String status) {`
`41`	`41`	`Preconditions.checkArgument(status != null, "Status is null");`
`42`	`42`	`try {`
`43`		`- return PlanStatus.valueOf(status.toUpperCase(Locale.ENGLISH));`
	`43`	`+ return PlanStatus.valueOf(status.toUpperCase(Locale.ROOT));`
`44`	`44`	`} catch (IllegalArgumentException e) {`
`45`	`45`	`throw new IllegalArgumentException(String.format("Invalid status name: %s", status), e);`
`46`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -83,6 +83,7 @@ private PlanTableScanRequest(`
`83`	`83`	`this.startSnapshotId = startSnapshotId;`
`84`	`84`	`this.endSnapshotId = endSnapshotId;`
`85`	`85`	`this.statsFields = statsFields;`
	`86`	`+ validate();`
`86`	`87`	`}`
`87`	`88`
`88`	`89`	`@Override`