laserninja commented on code in PR #12194:
URL: https://github.com/apache/gravitino/pull/12194#discussion_r3668835250
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -484,6 +492,46 @@ public PlanTableScanResponse planTableScan(
}
}
+ /**
+ * Fetch the scan tasks associated with a {@code plan-task} token previously
handed out by {@link
+ * #planTableScan}.
+ *
+ * <p>Scan planning here is synchronous: {@link #planTableScan} always
returns {@code COMPLETED}
+ * with the full set of {@code file-scan-tasks} inline and never emits
{@code plan-tasks} tokens.
+ * A client therefore has no token to present, and any token reaching this
method did not
+ * originate from this server (or came from a server generation that no
longer retains it). Per
+ * the Iceberg REST specification that case is an unknown plan task, so this
method always throws
+ * {@link NoSuchPlanTaskException}, which the REST layer maps to 404.
+ *
+ * <p>The endpoint still exists and is advertised in {@code /v1/config}
because clients such as
+ * pyiceberg refuse to use server-side scan planning at all unless {@code
POST
+ * .../tables/{table}/tasks} is advertised as supported. Implementing it
keeps the two-step
+ * protocol contract intact and gives batched planning a place to land if
{@link #planTableScan}
+ * later starts emitting {@code plan-tasks}.
+ *
+ * @param tableIdentifier the table the plan task belongs to.
+ * @param request the request carrying the {@code plan-task} token.
+ * @return never returns normally.
+ * @throws org.apache.iceberg.exceptions.NoSuchTableException if the table
doesn't exist.
+ * @throws NoSuchPlanTaskException always, since no {@code plan-task} tokens
are ever issued.
+ */
+ public FetchScanTasksResponse fetchScanTasks(
+ TableIdentifier tableIdentifier, FetchScanTasksRequest request) {
+ // Validate the table exists first, so a bad table reports 404 for the
table rather than
+ // masking it as an unknown plan task. Consistent with planTableScan
behavior.
+ getCatalog().loadTable(tableIdentifier);
+
+ LOG.info(
+ "Rejecting unknown plan task '{}' for table {}: scan planning is
synchronous and does not "
+ + "issue plan-task tokens.",
+ request.planTask(),
+ tableIdentifier);
+ throw new NoSuchPlanTaskException(
Review Comment:
Correction to something I claimed earlier in this PR: I said scan responses
had to list the delete files their tasks reference, and that a merge-on-read
plan could not be serialized without it. That is wrong, and the extra code is
now removed (3e239e24f).
Iceberg 1.11's `BaseScanTaskResponse.Builder.withFileScanTasks` already
derives the response's `delete-files` from the tasks it is given, which is
exactly why `withDeleteFiles` is deprecated. I verified it by serializing a
plan of a table with a position delete file without touching delete files at
all: the response carries `"delete-files":[…]` and
`"delete-file-references":[0]` either way. So `POST .../plan` was never broken
for merge-on-read tables, and the deprecated calls I had added were redundant.
What batching does still have to guarantee is that a task and the delete
files it references end up in the *same* response, since the references are
indexes into that response. That holds because each batch is built through
`withFileScanTasks(batch)`, and the test now asserts it for a fetched batch
instead of restating Iceberg's behaviour. The PR description is updated to drop
the incorrect claim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
