This is an automated email from the ASF dual-hosted git repository.
etudenhoefner pushed a commit to branch 1.10.x
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/1.10.x by this push:
new be18b36326 [1.10.x] Core: Fix planning request/response validations
(#14566)
be18b36326 is described below
commit be18b36326896884a3e50afa3715e650c22eb616
Author: Eduard Tudenhoefner <[email protected]>
AuthorDate: Wed Nov 12 09:22:23 2025 +0100
[1.10.x] Core: Fix planning request/response validations (#14566)
* Core: Fix validation of PlanTableScanRequest (#14561)
The OpenAPI spec says in
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L618-L624
that if no snapshotId is provided, the server can select the latest
snapshot in the table's main branch. However, currently the request always
requires to set either the `snapshotId` or the `startSnapshotId`/`endSnapshotId`
* Core: Fix PlanTableScanResponse validation (#14562)
The OpenAPI spec indicates in
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L3376-L3384
that a `completed` planning result can have a `planId`
---
.../rest/requests/PlanTableScanRequest.java | 23 +++++++-
.../rest/requests/PlanTableScanRequestParser.java | 2 +-
.../rest/responses/PlanTableScanResponse.java | 27 +++++++--
...st.java => TestPlanTableScanRequestParser.java} | 56 +++++++++++++++---
.../responses/TestPlanTableScanResponseParser.java | 67 ++++++++++++++++------
5 files changed, 142 insertions(+), 33 deletions(-)
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 14e14eab4b..97eeb56fde 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
@@ -88,9 +88,17 @@ public class PlanTableScanRequest implements RESTRequest {
@Override
public void validate() {
- Preconditions.checkArgument(
- snapshotId != null ^ (startSnapshotId != null && endSnapshotId !=
null),
- "Either snapshotId must be provided or both startSnapshotId and
endSnapshotId must be provided");
+ if (null != snapshotId) {
+ Preconditions.checkArgument(
+ null == startSnapshotId && null == endSnapshotId,
+ "Invalid scan: cannot provide both snapshotId and
startSnapshotId/endSnapshotId");
+ }
+
+ if (null != startSnapshotId || null != endSnapshotId) {
+ Preconditions.checkArgument(
+ null != startSnapshotId && null != endSnapshotId,
+ "Invalid incremental scan: startSnapshotId and endSnapshotId is
required");
+ }
}
@Override
@@ -107,6 +115,10 @@ public class PlanTableScanRequest implements RESTRequest {
.toString();
}
+ public static Builder builder() {
+ return new Builder();
+ }
+
public static class Builder {
private Long snapshotId;
private List<String> select;
@@ -117,6 +129,11 @@ public class PlanTableScanRequest implements RESTRequest {
private Long endSnapshotId;
private List<String> statsFields;
+ /**
+ * @deprecated since 1.11.0, visibility will be reduced in 1.12.0; use
{@link
+ * PlanTableScanRequest#builder()} instead.
+ */
+ @Deprecated
public Builder() {}
public Builder withSnapshotId(Long withSnapshotId) {
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 9b2eb9adb4..6801330466 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
@@ -121,7 +121,7 @@ public class PlanTableScanRequestParser {
List<String> statsFields = JsonUtil.getStringListOrNull(STATS_FIELDS,
json);
- return new PlanTableScanRequest.Builder()
+ return PlanTableScanRequest.builder()
.withSnapshotId(snapshotId)
.withSelect(select)
.withFilter(filter)
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 4596f8d5cd..95f862de96 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
@@ -66,16 +66,24 @@ public class PlanTableScanResponse extends
BaseScanTaskResponse {
planStatus() != null, "Invalid response: plan status must be defined");
Preconditions.checkArgument(
planStatus() != PlanStatus.SUBMITTED || planId() != null,
- "Invalid response: plan id should be defined when status is
'submitted'");
+ "Invalid response: plan id should be defined when status is '%s'",
+ PlanStatus.SUBMITTED.status());
Preconditions.checkArgument(
planStatus() != PlanStatus.CANCELLED,
- "Invalid response: 'cancelled' is not a valid status for
planTableScan");
+ "Invalid response: '%s' is not a valid status for planTableScan",
+ PlanStatus.CANCELLED.status());
Preconditions.checkArgument(
planStatus() == PlanStatus.COMPLETED || (planTasks() == null &&
fileScanTasks() == null),
- "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 defined when status is
'submitted'");
+ "Invalid response: tasks can only be defined when status is '%s'",
+ PlanStatus.COMPLETED.status());
+ if (null != planId()) {
+ Preconditions.checkArgument(
+ planStatus() == PlanStatus.SUBMITTED || planStatus() ==
PlanStatus.COMPLETED,
+ "Invalid response: plan id can only be defined when status is '%s'
or '%s'",
+ PlanStatus.SUBMITTED.status(),
+ PlanStatus.COMPLETED.status());
+ }
+
if (fileScanTasks() == null || fileScanTasks().isEmpty()) {
Preconditions.checkArgument(
(deleteFiles() == null || deleteFiles().isEmpty()),
@@ -91,6 +99,13 @@ public class PlanTableScanResponse extends
BaseScanTaskResponse {
private PlanStatus planStatus;
private String planId;
+ /**
+ * @deprecated since 1.11.0, visibility will be reduced in 1.12.0; use
{@link
+ * PlanTableScanResponse#builder()} instead.
+ */
+ @Deprecated
+ public Builder() {}
+
public Builder withPlanStatus(PlanStatus status) {
this.planStatus = status;
return this;
diff --git
a/core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequest.java
b/core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequestParser.java
similarity index 69%
rename from
core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequest.java
rename to
core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequestParser.java
index f18928a1a3..55929d53d3 100644
---
a/core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequest.java
+++
b/core/src/test/java/org/apache/iceberg/rest/requests/TestPlanTableScanRequestParser.java
@@ -26,7 +26,7 @@ import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.junit.jupiter.api.Test;
-public class TestPlanTableScanRequest {
+public class TestPlanTableScanRequestParser {
@Test
public void nullAndEmptyCheck() {
@@ -39,10 +39,52 @@ public class TestPlanTableScanRequest {
.hasMessage("Invalid planTableScanRequest: null");
}
+ @Test
+ public void requestWithValidSnapshotIds() {
+ PlanTableScanRequest request = PlanTableScanRequest.builder().build();
+ assertThat(request).isNotNull();
+ assertThat(request.snapshotId()).isNull();
+ assertThat(request.startSnapshotId()).isNull();
+ assertThat(request.endSnapshotId()).isNull();
+
+ request = PlanTableScanRequest.builder().withSnapshotId(1L).build();
+ assertThat(request.snapshotId()).isEqualTo(1L);
+ assertThat(request.startSnapshotId()).isNull();
+ assertThat(request.endSnapshotId()).isNull();
+
+ request =
PlanTableScanRequest.builder().withStartSnapshotId(1L).withEndSnapshotId(5L).build();
+ assertThat(request.snapshotId()).isNull();
+ assertThat(request.startSnapshotId()).isEqualTo(1L);
+ assertThat(request.endSnapshotId()).isEqualTo(5);
+ }
+
+ @Test
+ public void requestWithInvalidSnapshotIds() {
+ assertThatThrownBy(
+ () ->
PlanTableScanRequest.builder().withSnapshotId(1L).withStartSnapshotId(1L).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(
+ "Invalid scan: cannot provide both snapshotId and
startSnapshotId/endSnapshotId");
+
+ assertThatThrownBy(
+ () ->
PlanTableScanRequest.builder().withSnapshotId(1L).withEndSnapshotId(5L).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(
+ "Invalid scan: cannot provide both snapshotId and
startSnapshotId/endSnapshotId");
+
+ assertThatThrownBy(() ->
PlanTableScanRequest.builder().withStartSnapshotId(1L).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid incremental scan: startSnapshotId and
endSnapshotId is required");
+
+ assertThatThrownBy(() ->
PlanTableScanRequest.builder().withEndSnapshotId(5L).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid incremental scan: startSnapshotId and
endSnapshotId is required");
+ }
+
@Test
public void roundTripSerdeWithSelectField() {
PlanTableScanRequest request =
- new PlanTableScanRequest.Builder()
+ PlanTableScanRequest.builder()
.withSnapshotId(1L)
.withSelect(Lists.newArrayList("col1", "col2"))
.build();
@@ -62,7 +104,7 @@ public class TestPlanTableScanRequest {
@Test
public void roundTripSerdeWithFilterField() {
PlanTableScanRequest request =
- new PlanTableScanRequest.Builder()
+ PlanTableScanRequest.builder()
.withSnapshotId(1L)
.withFilter(Expressions.alwaysFalse())
.build();
@@ -83,7 +125,7 @@ public class TestPlanTableScanRequest {
public void planTableScanRequestWithAllFieldsInvalidRequest() {
assertThatThrownBy(
() ->
- new PlanTableScanRequest.Builder()
+ PlanTableScanRequest.builder()
.withSnapshotId(1L)
.withSelect(Lists.newArrayList("col1", "col2"))
.withFilter(Expressions.alwaysTrue())
@@ -95,13 +137,13 @@ public class TestPlanTableScanRequest {
.build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
- "Either snapshotId must be provided or both startSnapshotId and
endSnapshotId must be provided");
+ "Invalid scan: cannot provide both snapshotId and
startSnapshotId/endSnapshotId");
}
@Test
public void roundTripSerdeWithAllFieldsExceptSnapShotId() {
PlanTableScanRequest request =
- new PlanTableScanRequest.Builder()
+ PlanTableScanRequest.builder()
.withSelect(Lists.newArrayList("col1", "col2"))
.withFilter(Expressions.alwaysTrue())
.withStartSnapshotId(1L)
@@ -129,7 +171,7 @@ public class TestPlanTableScanRequest {
@Test
public void testToStringContainsAllFields() {
PlanTableScanRequest request =
- new PlanTableScanRequest.Builder()
+ PlanTableScanRequest.builder()
.withSnapshotId(123L)
.withSelect(Lists.newArrayList("colA", "colB"))
.withFilter(Expressions.alwaysTrue())
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 49c0ad1fa0..c807bc4655 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
@@ -53,7 +53,6 @@ public class TestPlanTableScanResponseParser {
@Test
public void roundTripSerdeWithEmptyObject() {
-
assertThatThrownBy(() -> PlanTableScanResponse.builder().build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid response: plan status must be defined");
@@ -65,6 +64,45 @@ public class TestPlanTableScanResponseParser {
.hasMessage("Cannot parse planTableScan response from empty or null
object");
}
+ @Test
+ public void roundTripSerdeWithCompletedPlanningWithAndWithoutPlanId() {
+ PlanTableScanResponse response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .build();
+ assertThat(response.planStatus()).isEqualTo(PlanStatus.COMPLETED);
+ assertThat(response.planId()).isNull();
+ assertThat(PlanTableScanResponseParser.toJson(response))
+ .isEqualTo("{\"plan-status\":\"completed\"}");
+
+ response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withPlanId("somePlanId")
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .build();
+ assertThat(response.planStatus()).isEqualTo(PlanStatus.COMPLETED);
+ assertThat(response.planId()).isEqualTo("somePlanId");
+
+ assertThat(PlanTableScanResponseParser.toJson(response))
+
.isEqualTo("{\"plan-status\":\"completed\",\"plan-id\":\"somePlanId\"}");
+ }
+
+ @Test
+ public void roundTripSerdeWithSubmittedPlanningWithPlanId() {
+ PlanTableScanResponse response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.SUBMITTED)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .withPlanId("somePlanId")
+ .build();
+ assertThat(response.planStatus()).isEqualTo(PlanStatus.SUBMITTED);
+ assertThat(response.planId()).isEqualTo("somePlanId");
+ assertThat(PlanTableScanResponseParser.toJson(response))
+
.isEqualTo("{\"plan-status\":\"submitted\",\"plan-id\":\"somePlanId\"}");
+ }
+
@Test
public void roundTripSerdeWithInvalidPlanStatus() {
String invalidStatusJson = "{\"plan-status\": \"someStatus\"}";
@@ -78,9 +116,8 @@ public class TestPlanTableScanResponseParser {
@Test
public void roundTripSerdeWithInvalidPlanStatusSubmittedWithoutPlanId() {
- PlanStatus planStatus = PlanStatus.fromName("submitted");
-
- assertThatThrownBy(() ->
PlanTableScanResponse.builder().withPlanStatus(planStatus).build())
+ assertThatThrownBy(
+ () ->
PlanTableScanResponse.builder().withPlanStatus(PlanStatus.SUBMITTED).build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid response: plan id should be defined when status
is 'submitted'");
@@ -93,8 +130,8 @@ public class TestPlanTableScanResponseParser {
@Test
public void roundTripSerdeWithInvalidPlanStatusCancelled() {
- PlanStatus planStatus = PlanStatus.fromName("cancelled");
- assertThatThrownBy(() ->
PlanTableScanResponse.builder().withPlanStatus(planStatus).build())
+ assertThatThrownBy(
+ () ->
PlanTableScanResponse.builder().withPlanStatus(PlanStatus.CANCELLED).build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid response: 'cancelled' is not a valid status for
planTableScan");
@@ -107,11 +144,10 @@ public class TestPlanTableScanResponseParser {
@Test
public void roundTripSerdeWithInvalidPlanStatusSubmittedWithTasksPresent() {
- PlanStatus planStatus = PlanStatus.fromName("submitted");
assertThatThrownBy(
() ->
PlanTableScanResponse.builder()
- .withPlanStatus(planStatus)
+ .withPlanStatus(PlanStatus.SUBMITTED)
.withPlanId("somePlanId")
.withPlanTasks(List.of("task1", "task2"))
.build())
@@ -131,31 +167,31 @@ public class TestPlanTableScanResponseParser {
@Test
public void roundTripSerdeWithInvalidPlanIdWithIncorrectStatus() {
- PlanStatus planStatus = PlanStatus.fromName("failed");
assertThatThrownBy(
() ->
PlanTableScanResponse.builder()
- .withPlanStatus(planStatus)
+ .withPlanStatus(PlanStatus.FAILED)
.withPlanId("somePlanId")
.build())
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid response: plan id can only be defined when status
is 'submitted'");
+ .hasMessage(
+ "Invalid response: plan id can only be defined when status is
'submitted' or 'completed'");
String invalidJson = "{\"plan-status\":\"failed\"," +
"\"plan-id\":\"somePlanId\"}";
assertThatThrownBy(
() -> PlanTableScanResponseParser.fromJson(invalidJson,
PARTITION_SPECS_BY_ID, false))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid response: plan id can only be defined when status
is 'submitted'");
+ .hasMessage(
+ "Invalid response: plan id can only be defined when status is
'submitted' or 'completed'");
}
@Test
public void
roundTripSerdeWithInvalidPlanStatusSubmittedWithDeleteFilesNoFileScanTasksPresent()
{
- PlanStatus planStatus = PlanStatus.fromName("submitted");
assertThatThrownBy(
() ->
PlanTableScanResponse.builder()
- .withPlanStatus(planStatus)
+ .withPlanStatus(PlanStatus.SUBMITTED)
.withPlanId("somePlanId")
.withDeleteFiles(List.of(FILE_A_DELETES))
.build())
@@ -190,10 +226,9 @@ public class TestPlanTableScanResponseParser {
PartitionSpecParser.toJson(SPEC),
residualEvaluator);
- PlanStatus planStatus = PlanStatus.fromName("completed");
PlanTableScanResponse response =
PlanTableScanResponse.builder()
- .withPlanStatus(planStatus)
+ .withPlanStatus(PlanStatus.COMPLETED)
.withFileScanTasks(List.of(fileScanTask))
.withDeleteFiles(List.of(FILE_A_DELETES))
.withSpecsById(PARTITION_SPECS_BY_ID)
