Skip to content

Commit 050ef6a

Browse files
sfc-gh-prsinghsfc-gh-prsingh
committed
Address second round review feedback
1 parent 64d74dc commit 050ef6a

15 files changed

+91
-59
lines changed

‎core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.fasterxml.jackson.core.JsonGenerator;
2222
import com.fasterxml.jackson.databind.JsonNode;
2323
import java.io.IOException;
24+
import java.util.Collections;
2425
import java.util.List;
2526
import java.util.Map;
2627
import java.util.Set;
@@ -77,6 +78,11 @@ public static FileScanTask fromJson(
7778
DeleteFile[] deleteFiles = null;
7879
if (jsonNode.has(DELETE_FILE_REFERENCES)) {
7980
List<Integer> indices = JsonUtil.getIntegerList(DELETE_FILE_REFERENCES, jsonNode);
81+
Preconditions.checkArgument(
82+
Collections.max(indices) < allDeleteFiles.size(),
83+
"Invalid delete file references: %s, expected indices < %s",
84+
indices,
85+
allDeleteFiles.size());
8086
deleteFiles =
8187
indices.stream()
8288
.map(index -> (GenericDeleteFile) allDeleteFiles.get(index))

‎core/src/main/java/org/apache/iceberg/TableScanResponseParser.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public static List<DeleteFile> parseDeleteFiles(
5252
return deleteFilesBuilder.build();
5353
}
5454

55-
return null;
55+
return Lists.newArrayList();
5656
}
5757

5858
public static List<FileScanTask> parseFileScanTasks(
@@ -85,7 +85,7 @@ public static void serializeScanTasks(
8585
JsonGenerator gen)
8686
throws IOException {
8787
Map<String, Integer> deleteFilePathToIndex = Maps.newHashMap();
88-
if (deleteFiles != null) {
88+
if (deleteFiles != null && !deleteFiles.isEmpty()) {
8989
Preconditions.checkArgument(
9090
specsById != null, "Cannot serialize response without specs by ID defined");
9191
gen.writeArrayFieldStart(DELETE_FILES);

‎core/src/main/java/org/apache/iceberg/rest/PlanStatus.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public String status() {
4040
public static PlanStatus fromName(String status) {
4141
Preconditions.checkArgument(status != null, "Status is null");
4242
try {
43-
return PlanStatus.valueOf(status.toUpperCase(Locale.ENGLISH));
43+
return PlanStatus.valueOf(status.toUpperCase(Locale.ROOT));
4444
} catch (IllegalArgumentException e) {
4545
throw new IllegalArgumentException(String.format("Invalid status name: %s", status), e);
4646
}

‎core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java‎

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -545,13 +545,11 @@ static class PlanTableScanResponseDeserializer<T extends PlanTableScanResponse>
545545
@Override
546546
public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
547547
JsonNode jsonNode = p.getCodec().readTree(p);
548-
// Retrieve injectable values
549-
@SuppressWarnings("unchecked")
550-
Map<Integer, PartitionSpec> specsById =
551-
(Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
548+
TableScanResponseContext scanContext = parseScanResponseContext(context);
552549

553-
boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
554-
return (T) PlanTableScanResponseParser.fromJson(jsonNode, specsById, caseSensitive);
550+
return (T)
551+
PlanTableScanResponseParser.fromJson(
552+
jsonNode, scanContext.getSpecsById(), scanContext.isCaseSensitive());
555553
}
556554
}
557555

@@ -569,13 +567,11 @@ static class FetchPlanningResultResponseDeserializer<T extends FetchPlanningResu
569567
@Override
570568
public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
571569
JsonNode jsonNode = p.getCodec().readTree(p);
572-
// Retrieve injectable values
573-
@SuppressWarnings("unchecked")
574-
Map<Integer, PartitionSpec> specsById =
575-
(Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
576570

577-
boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
578-
return (T) FetchPlanningResultResponseParser.fromJson(jsonNode, specsById, caseSensitive);
571+
TableScanResponseContext scanContext = parseScanResponseContext(context);
572+
return (T)
573+
FetchPlanningResultResponseParser.fromJson(
574+
jsonNode, scanContext.getSpecsById(), scanContext.isCaseSensitive());
579575
}
580576
}
581577

@@ -593,13 +589,38 @@ static class FetchScanTaskResponseDeserializer<T extends FetchScanTasksResponse>
593589
@Override
594590
public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
595591
JsonNode jsonNode = p.getCodec().readTree(p);
596-
// Retrieve injectable values
597-
@SuppressWarnings("unchecked")
598-
Map<Integer, PartitionSpec> specsById =
599-
(Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
600592

601-
boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
602-
return (T) FetchScanTasksResponseParser.fromJson(jsonNode, specsById, caseSensitive);
593+
TableScanResponseContext scanContext = parseScanResponseContext(context);
594+
return (T)
595+
FetchScanTasksResponseParser.fromJson(
596+
jsonNode, scanContext.getSpecsById(), scanContext.isCaseSensitive());
597+
}
598+
}
599+
600+
private static TableScanResponseContext parseScanResponseContext(DeserializationContext context)
601+
throws IOException {
602+
@SuppressWarnings("unchecked")
603+
Map<Integer, PartitionSpec> specsById =
604+
(Map<Integer, PartitionSpec>) context.findInjectableValue("specsById", null, null);
605+
boolean caseSensitive = (boolean) context.findInjectableValue("caseSensitive", null, null);
606+
return new TableScanResponseContext(specsById, caseSensitive);
607+
}
608+
609+
static class TableScanResponseContext {
610+
private final Map<Integer, PartitionSpec> specsById;
611+
private final boolean caseSensitive;
612+
613+
TableScanResponseContext(Map<Integer, PartitionSpec> specs, boolean caseSen) {
614+
this.specsById = specs;
615+
this.caseSensitive = caseSen;
616+
}
617+
618+
Map<Integer, PartitionSpec> getSpecsById() {
619+
return specsById;
620+
}
621+
622+
boolean isCaseSensitive() {
623+
return caseSensitive;
603624
}
604625
}
605626
}

‎core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ private PlanTableScanRequest(
8383
this.startSnapshotId = startSnapshotId;
8484
this.endSnapshotId = endSnapshotId;
8585
this.statsFields = statsFields;
86+
validate();
8687
}
8788

8889
@Override

core/src/main/java/org/apache/iceberg/rest/responses/BaseScanResponse.java renamed to core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,14 @@
2525
import org.apache.iceberg.PartitionSpec;
2626
import org.apache.iceberg.rest.RESTResponse;
2727

28-
public abstract class BaseScanResponse implements RESTResponse {
28+
public abstract class BaseScanTaskResponse implements RESTResponse {
2929

3030
private final List<String> planTasks;
3131
private final List<FileScanTask> fileScanTasks;
3232
private final List<DeleteFile> deleteFiles;
3333
private final Map<Integer, PartitionSpec> specsById;
3434

35-
protected BaseScanResponse(
35+
protected BaseScanTaskResponse(
3636
List<String> planTasks,
3737
List<FileScanTask> fileScanTasks,
3838
List<DeleteFile> deleteFiles,
@@ -59,7 +59,7 @@ public Map<Integer, PartitionSpec> specsById() {
5959
return specsById;
6060
}
6161

62-
public abstract static class Builder<B extends Builder<B, R>, R extends BaseScanResponse> {
62+
public abstract static class Builder<B extends Builder<B, R>, R extends BaseScanTaskResponse> {
6363
private List<String> planTasks;
6464
private List<FileScanTask> fileScanTasks;
6565
private List<DeleteFile> deleteFiles;

‎core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
2727
import org.apache.iceberg.rest.PlanStatus;
2828

29-
public class FetchPlanningResultResponse extends BaseScanResponse {
29+
public class FetchPlanningResultResponse extends BaseScanTaskResponse {
3030
private final PlanStatus planStatus;
3131

3232
private FetchPlanningResultResponse(
@@ -62,7 +62,7 @@ public void validate() {
6262
}
6363

6464
public static class Builder
65-
extends BaseScanResponse.Builder<Builder, FetchPlanningResultResponse> {
65+
extends BaseScanTaskResponse.Builder<Builder, FetchPlanningResultResponse> {
6666
private Builder() {}
6767

6868
private PlanStatus planStatus;

‎core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponseParser.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.apache.iceberg.FileScanTask;
2828
import org.apache.iceberg.PartitionSpec;
2929
import org.apache.iceberg.TableScanResponseParser;
30+
import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
3031
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
3132
import org.apache.iceberg.rest.PlanStatus;
3233
import org.apache.iceberg.util.JsonUtil;
@@ -63,9 +64,11 @@ public static void toJson(FetchPlanningResultResponse response, JsonGenerator ge
6364
gen.writeEndObject();
6465
}
6566

66-
public static FetchPlanningResultResponse fromJson(
67+
@VisibleForTesting
68+
static FetchPlanningResultResponse fromJson(
6769
String json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
68-
Preconditions.checkArgument(json != null, "Invalid fetchPlanningResult response: null");
70+
Preconditions.checkArgument(
71+
json != null, "Invalid fetchPlanningResult response: null or empty");
6972
return JsonUtil.parse(
7073
json,
7174
node -> {

‎core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponse.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import org.apache.iceberg.PartitionSpec;
2626
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
2727

28-
public class FetchScanTasksResponse extends BaseScanResponse {
28+
public class FetchScanTasksResponse extends BaseScanTaskResponse {
2929

3030
private FetchScanTasksResponse(
3131
List<String> planTasks,
@@ -53,7 +53,8 @@ public static Builder builder() {
5353
return new Builder();
5454
}
5555

56-
public static class Builder extends BaseScanResponse.Builder<Builder, FetchScanTasksResponse> {
56+
public static class Builder
57+
extends BaseScanTaskResponse.Builder<Builder, FetchScanTasksResponse> {
5758
private Builder() {}
5859

5960
@Override

‎core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponseParser.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.apache.iceberg.FileScanTask;
2828
import org.apache.iceberg.PartitionSpec;
2929
import org.apache.iceberg.TableScanResponseParser;
30+
import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
3031
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
3132
import org.apache.iceberg.util.JsonUtil;
3233

@@ -59,7 +60,8 @@ public static void toJson(FetchScanTasksResponse response, JsonGenerator gen) th
5960
gen.writeEndObject();
6061
}
6162

62-
public static FetchScanTasksResponse fromJson(
63+
@VisibleForTesting
64+
static FetchScanTasksResponse fromJson(
6365
String json, Map<Integer, PartitionSpec> specsById, boolean caseSensitive) {
6466
Preconditions.checkArgument(json != null, "Cannot parse fetchScanTasks response from null");
6567
return JsonUtil.parse(

0 commit comments

Comments
 (0)