diff --git a/core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java b/core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java index 7f2ab833c7b0..d1ff4a738ae9 100644 --- a/core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java +++ b/core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java @@ -49,18 +49,8 @@ public static void toJson( generator.writeStartObject(); generator.writeFieldName(DATA_FILE); ContentFileParser.unboundContentFileToJson(fileScanTask.file(), partitionSpec, generator); - if (deleteFileReferences != null) { - generator.writeArrayFieldStart(DELETE_FILE_REFERENCES); - deleteFileReferences.forEach( - delIdx -> { - try { - generator.writeNumber(delIdx); - } catch (IOException e) { - throw new RuntimeException(e); - } - }); - generator.writeEndArray(); + JsonUtil.writeIntegerArray(DELETE_FILE_REFERENCES, deleteFileReferences, generator); } if (fileScanTask.residual() != null) { @@ -92,8 +82,6 @@ public static FileScanTask fromJson(JsonNode jsonNode, List allDelet filter = ExpressionParser.fromJson(jsonNode.get(RESIDUAL)); } - // TODO at the time of creation we dont have the schemaString, specString, and residual so will - // need to bind later return new UnboundBaseFileScanTask(dataFile, deleteFiles, filter); } } diff --git a/core/src/main/java/org/apache/iceberg/UnboundBaseFileScanTask.java b/core/src/main/java/org/apache/iceberg/UnboundBaseFileScanTask.java index 14fc1b21f14e..b8625e86bb2a 100644 --- a/core/src/main/java/org/apache/iceberg/UnboundBaseFileScanTask.java +++ b/core/src/main/java/org/apache/iceberg/UnboundBaseFileScanTask.java @@ -20,7 +20,6 @@ import org.apache.iceberg.expressions.Expression; import org.apache.iceberg.expressions.ResidualEvaluator; -import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; public class UnboundBaseFileScanTask extends BaseFileScanTask { private DataFile unboundDataFile; @@ -45,21 +44,7 @@ public PartitionSpec spec() { throw new UnsupportedOperationException("spec() is not supported in UnboundBaseFileScanTask"); } - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("unboundDataFile", unboundDataFile) - .add("unboundDeleteFiles", unboundDeleteFiles) - .add("filter", filter) - .toString(); - } - public FileScanTask bind(PartitionSpec spec, boolean caseSensitive) { - // TODO before creating a new task - // need to ensure that dataFile is refreshed with correct partitionData using spec - // need to ensure deleteFiles is refreshed with spec info - // need to ensure residual refreshed with spec. - Metrics dataFileMetrics = new Metrics( unboundDataFile.recordCount(), diff --git a/core/src/main/java/org/apache/iceberg/rest/requests/FetchScanTasksRequest.java b/core/src/main/java/org/apache/iceberg/rest/requests/FetchScanTasksRequest.java index c0c398304299..2293baac999e 100644 --- a/core/src/main/java/org/apache/iceberg/rest/requests/FetchScanTasksRequest.java +++ b/core/src/main/java/org/apache/iceberg/rest/requests/FetchScanTasksRequest.java @@ -24,7 +24,7 @@ public class FetchScanTasksRequest implements RESTRequest { - private String planTask; + private final String planTask; public FetchScanTasksRequest(String planTask) { this.planTask = planTask; @@ -37,7 +37,7 @@ public String planTask() { @Override public void validate() { - Preconditions.checkArgument(planTask != null, "Invalid request: planTask null"); + Preconditions.checkArgument(planTask != null, "Invalid planTask: null"); } @Override 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 index a3955a5ef399..d85ee324b0dd 100644 --- a/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java +++ b/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java @@ -21,17 +21,18 @@ import java.util.List; import org.apache.iceberg.expressions.Expression; import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.rest.RESTRequest; public class PlanTableScanRequest implements RESTRequest { - private Long snapshotId; - private List select; - private Expression filter; - private Boolean caseSensitive; - private Boolean useSnapshotSchema; - private Long startSnapshotId; - private Long endSnapshotId; - private List statsFields; + private final Long snapshotId; + private final List select; + private final Expression filter; + private final boolean caseSensitive; + private final boolean useSnapshotSchema; + private final Long startSnapshotId; + private final Long endSnapshotId; + private final List statsFields; public Long snapshotId() { return snapshotId; @@ -86,20 +87,21 @@ private PlanTableScanRequest( @Override public void validate() { - // validation logic to be performed in PlanTableScanRequestParser + if (snapshotId != null || startSnapshotId != null || endSnapshotId != null) { + Preconditions.checkArgument( + snapshotId != null ^ (startSnapshotId != null && endSnapshotId != null), + "Either snapshotId must be provided or both startSnapshotId and endSnapshotId must be provided"); + } } @Override public String toString() { return MoreObjects.toStringHelper(this) .add("snapshotId", snapshotId) - .add("select", select) - .add("filter", filter) .add("caseSensitive", caseSensitive) .add("useSnapshotSchema", useSnapshotSchema) .add("startSnapshotId", startSnapshotId) .add("endSnapshotId", endSnapshotId) - .add("statsFields", statsFields) .toString(); } @@ -120,38 +122,38 @@ public Builder withSnapshotId(Long withSnapshotId) { return this; } - public Builder withSelect(List withSelect) { - this.select = withSelect; + public Builder withSelect(List projection) { + this.select = projection; return this; } - public Builder withFilter(Expression withFilter) { - this.filter = withFilter; + public Builder withFilter(Expression expression) { + this.filter = expression; return this; } - public Builder withCaseSensitive(boolean withCaseSensitive) { - this.caseSensitive = withCaseSensitive; + public Builder withCaseSensitive(boolean value) { + this.caseSensitive = value; return this; } - public Builder withUseSnapshotSchema(boolean withUseSnapshotSchema) { - this.useSnapshotSchema = withUseSnapshotSchema; + public Builder withUseSnapshotSchema(boolean snapshotSchema) { + this.useSnapshotSchema = snapshotSchema; return this; } - public Builder withStartSnapshotId(Long withStartSnapshotId) { - this.startSnapshotId = withStartSnapshotId; + public Builder withStartSnapshotId(Long startingSnapshotId) { + this.startSnapshotId = startingSnapshotId; return this; } - public Builder withEndSnapshotId(Long withEndSnapshotId) { - this.endSnapshotId = withEndSnapshotId; + public Builder withEndSnapshotId(Long endingSnapshotId) { + this.endSnapshotId = endingSnapshotId; return this; } - public Builder withStatsFields(List withStatsFields) { - this.statsFields = withStatsFields; + public Builder withStatsFields(List fields) { + this.statsFields = fields; return this; } diff --git a/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequestParser.java b/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequestParser.java index cb9666e1581c..e840841fcfcf 100644 --- a/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequestParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequestParser.java @@ -102,26 +102,19 @@ public static PlanTableScanRequest fromJson(JsonNode json) { Long startSnapshotId = JsonUtil.getLongOrNull(START_SNAPSHOT_ID, json); Long endSnapshotId = JsonUtil.getLongOrNull(END_SNAPSHOT_ID, json); - if (snapshotId != null || startSnapshotId != null || endSnapshotId != null) { - Preconditions.checkArgument( - snapshotId != null ^ (startSnapshotId != null && endSnapshotId != null), - "Either snapshotId must be provided or both startSnapshotId and endSnapshotId must be provided"); - } - List select = JsonUtil.getStringListOrNull(SELECT, json); Expression filter = null; if (json.has(FILTER)) { - // TODO without text value it adds another " " filter = ExpressionParser.fromJson(json.get(FILTER).textValue()); } - Boolean caseSensitive = true; + boolean caseSensitive = true; if (json.has(CASE_SENSITIVE)) { caseSensitive = JsonUtil.getBool(CASE_SENSITIVE, json); } - Boolean useSnapshotSchema = false; + boolean useSnapshotSchema = false; if (json.has(USE_SNAPSHOT_SCHEMA)) { useSnapshotSchema = JsonUtil.getBool(USE_SNAPSHOT_SCHEMA, json); } 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 index 34acbcfc4820..fd14be8c208d 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java @@ -28,14 +28,11 @@ import org.apache.iceberg.rest.RESTResponse; public class FetchPlanningResultResponse implements RESTResponse { - private PlanStatus planStatus; - - private List planTasks; - - private List fileScanTasks; - - private List deleteFiles; - private Map specsById; + private final PlanStatus planStatus; + private final List planTasks; + private final List fileScanTasks; + private final List deleteFiles; + private final Map specsById; private FetchPlanningResultResponse( PlanStatus planStatus, @@ -92,36 +89,33 @@ public static class Builder { private Builder() {} private PlanStatus planStatus; - private List planTasks; - private List fileScanTasks; - private List deleteFiles; private Map specsById; - public Builder withPlanStatus(PlanStatus withPlanStatus) { - this.planStatus = withPlanStatus; + public Builder withPlanStatus(PlanStatus status) { + this.planStatus = status; return this; } - public Builder withPlanTasks(List withPlanTasks) { - this.planTasks = withPlanTasks; + public Builder withPlanTasks(List tasks) { + this.planTasks = tasks; return this; } - public Builder withFileScanTasks(List withFileScanTasks) { - this.fileScanTasks = withFileScanTasks; + public Builder withFileScanTasks(List tasks) { + this.fileScanTasks = tasks; return this; } - public Builder withDeleteFiles(List withDeleteFiles) { - this.deleteFiles = withDeleteFiles; + public Builder withDeleteFiles(List deletes) { + this.deleteFiles = deletes; return this; } - public Builder withSpecsById(Map withSpecsById) { - this.specsById = withSpecsById; + public Builder withSpecsById(Map specs) { + this.specsById = specs; return this; } 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 index fb8dcf657e8a..26aaa59e8627 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponseParser.java @@ -46,6 +46,9 @@ public static void toJson(FetchPlanningResultResponse response, JsonGenerator ge throws IOException { Preconditions.checkArgument( null != response, "Invalid response: fetchPanningResultResponse null"); + Preconditions.checkArgument( + response.specsById() != null, + "Cannot serialize fetchingPlanningResultResponse without specsById"); gen.writeStartObject(); gen.writeStringField(PLAN_STATUS, response.planStatus().status()); if (response.planTasks() != null) { 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 index 3413c67ffe99..464dcc3f46c5 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponse.java @@ -27,10 +27,10 @@ import org.apache.iceberg.rest.RESTResponse; public class FetchScanTasksResponse implements RESTResponse { - private List planTasks; - private List fileScanTasks; - private List deleteFiles; - private Map specsById; + private final List planTasks; + private final List fileScanTasks; + private final List deleteFiles; + private final Map specsById; private FetchScanTasksResponse( List planTasks, @@ -85,23 +85,23 @@ private Builder() {} private List deleteFiles; private Map specsById; - public Builder withPlanTasks(List withPlanTasks) { - this.planTasks = withPlanTasks; + public Builder withPlanTasks(List tasks) { + this.planTasks = tasks; return this; } - public Builder withFileScanTasks(List withFileScanTasks) { - this.fileScanTasks = withFileScanTasks; + public Builder withFileScanTasks(List tasks) { + this.fileScanTasks = tasks; return this; } - public Builder withDeleteFiles(List withDeleteFiles) { - this.deleteFiles = withDeleteFiles; + public Builder withDeleteFiles(List deletes) { + this.deleteFiles = deletes; return this; } - public Builder withSpecsById(Map withSpecsById) { - this.specsById = withSpecsById; + public Builder withSpecsById(Map specs) { + this.specsById = specs; return this; } 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 index 60ebea6c85db..5011d0857d76 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/FetchScanTasksResponseParser.java @@ -42,6 +42,8 @@ public static String toJson(FetchScanTasksResponse response, boolean pretty) { public static void toJson(FetchScanTasksResponse response, JsonGenerator gen) throws IOException { Preconditions.checkArgument(response != null, "Invalid response: fetchScanTasksResponse null"); + Preconditions.checkArgument( + response.specsById() != null, "Cannot serialize fetchScanTasksResponse without specsById"); gen.writeStartObject(); if (response.planTasks() != null) { JsonUtil.writeStringArray(PLAN_TASKS, response.planTasks(), gen); 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 index d754bbcfe03c..57dd90d8d986 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java @@ -29,12 +29,12 @@ import org.apache.iceberg.rest.RESTResponse; public class PlanTableScanResponse implements RESTResponse { - private PlanStatus planStatus; - private String planId; - private List planTasks; - private List fileScanTasks; - private List deleteFiles; - private Map specsById; + private final PlanStatus planStatus; + private final String planId; + private final List planTasks; + private final List fileScanTasks; + private final List deleteFiles; + private final Map specsById; private PlanTableScanResponse( PlanStatus planStatus, @@ -121,33 +121,33 @@ private Builder() {} private List deleteFiles; private Map specsById; - public Builder withPlanStatus(PlanStatus withPlanStatus) { - this.planStatus = withPlanStatus; + public Builder withPlanStatus(PlanStatus status) { + this.planStatus = status; return this; } - public Builder withPlanId(String withPlanId) { - this.planId = withPlanId; + public Builder withPlanId(String id) { + this.planId = id; return this; } - public Builder withPlanTasks(List withPlanTasks) { - this.planTasks = withPlanTasks; + public Builder withPlanTasks(List tasks) { + this.planTasks = tasks; return this; } - public Builder withFileScanTasks(List withFileScanTasks) { - this.fileScanTasks = withFileScanTasks; + public Builder withFileScanTasks(List tasks) { + this.fileScanTasks = tasks; return this; } - public Builder withDeleteFiles(List withDeleteFiles) { - this.deleteFiles = withDeleteFiles; + public Builder withDeleteFiles(List deletes) { + this.deleteFiles = deletes; return this; } - public Builder withSpecsById(Map withSpecsById) { - this.specsById = withSpecsById; + public Builder withSpecsById(Map specs) { + this.specsById = specs; return this; } 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 index 9b1bea734465..25d8f11d9ac4 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java @@ -47,6 +47,8 @@ public static void toJson(PlanTableScanResponse response, JsonGenerator gen) thr Preconditions.checkArgument(null != response, "Invalid response: planTableScanResponse null"); Preconditions.checkArgument( response.planStatus() != null, "Invalid response: status can not be null"); + Preconditions.checkArgument( + response.specsById() != null, "Cannot serialize planTableScanResponse without specsById"); gen.writeStartObject(); gen.writeStringField(PLAN_STATUS, response.planStatus().status()); diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/TableScanResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/TableScanResponseParser.java index 619e8f993863..ac6dba3ba520 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/TableScanResponseParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/TableScanResponseParser.java @@ -106,8 +106,12 @@ public static void serializeScanTasks( } } - PartitionSpec partitionSpec = specsById.get(fileScanTask.file().specId()); - RESTFileScanTaskParser.toJson(fileScanTask, deleteFileReferences, partitionSpec, gen); + PartitionSpec spec = specsById.get(fileScanTask.file().specId()); + Preconditions.checkArgument( + spec != null, + "Cannot serialize scan task with unknown spec %s", + fileScanTask.file().specId()); + RESTFileScanTaskParser.toJson(fileScanTask, deleteFileReferences, spec, gen); } gen.writeEndArray(); } 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 893ad5d5de09..ef39e47bdc31 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 @@ -186,7 +186,6 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { .withPlanStatus(planStatus) .withFileScanTasks(List.of(fileScanTask)) .withDeleteFiles(List.of(FILE_A_DELETES)) - // assume you have set this already .withSpecsById(PARTITION_SPECS_BY_ID) .build(); @@ -203,11 +202,9 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { + "\"residual-filter\":{\"type\":\"eq\",\"term\":\"id\",\"value\":1}}]" + "}"; - String json = PlanTableScanResponseParser.toJson(response, false); + String json = PlanTableScanResponseParser.toJson(response); assertThat(json).isEqualTo(expectedToJson); - // make an unbound json where you expect to not have partitions for the data file, - // delete files as service does not send partition spec String expectedFromJson = "{\"plan-status\":\"completed\"," + "\"delete-files\":[{\"spec-id\":0,\"content\":\"POSITION_DELETES\"," @@ -222,7 +219,6 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { + "}"; PlanTableScanResponse fromResponse = PlanTableScanResponseParser.fromJson(json); - // Need to make a new response with partitionSpec set PlanTableScanResponse copyResponse = PlanTableScanResponse.builder() .withPlanStatus(fromResponse.planStatus()) @@ -235,6 +231,6 @@ public void roundTripSerdeWithValidStatusAndFileScanTasks() { // can't do an equality comparison on PlanTableScanRequest because we don't implement // equals/hashcode - assertThat(PlanTableScanResponseParser.toJson(copyResponse, false)).isEqualTo(expectedFromJson); + assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedFromJson); } }