laserninja commented on code in PR #11635:
URL: https://github.com/apache/gravitino/pull/11635#discussion_r3592963761
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -227,6 +229,32 @@ public PlanTableScanResponse planTableScan(
.planTableScan(tableIdentifier, scanRequest);
}
+ @Override
+ public FetchPlanningResultResponse fetchPlanningResult(
+ IcebergRequestContext context, TableIdentifier tableIdentifier, String
planId) {
+ // Validate the table exists first (consistent with planTableScan behavior)
+ icebergCatalogWrapperManager
+ .getCatalogWrapper(context.catalogName())
+ .getCatalog()
+ .loadTable(tableIdentifier);
+ throw new NoSuchPlanIdException(
Review Comment:
Yes, these need to be implemented here. `IcebergTableOperationExecutor` is
the leaf implementation of the `IcebergTableOperationDispatcher` interface. The
`IcebergTableEventDispatcher` and `IcebergTableHookDispatcher` decorators
delegate down to it, and this is the only layer with access to
`icebergCatalogWrapperManager` to validate the table exists before responding.
Since Gravitino only supports synchronous scan planning today
(`planTableScan` always returns `COMPLETED` and no plan IDs are retained),
there's no async plan to fetch or cancel, so the correct spec-compliant
behavior is to return 404. We still load the table first so a missing table
returns `NoSuchTableException` (404), consistent with `planTableScan`, per the
earlier review feedback.
The interface methods are abstract (not `default`), so every implementer
must provide them. If async planning is added later, this is the natural place
to wire in the real lookup.
Happy to add a `default` throwing implementation on the interface instead if
you'd prefer to slim down the executor, but that would lose the table-existence
validation.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -227,6 +229,32 @@ public PlanTableScanResponse planTableScan(
.planTableScan(tableIdentifier, scanRequest);
}
+ @Override
+ public FetchPlanningResultResponse fetchPlanningResult(
+ IcebergRequestContext context, TableIdentifier tableIdentifier, String
planId) {
+ // Validate the table exists first (consistent with planTableScan behavior)
+ icebergCatalogWrapperManager
+ .getCatalogWrapper(context.catalogName())
+ .getCatalog()
+ .loadTable(tableIdentifier);
+ throw new NoSuchPlanIdException(
+ "Plan %s not found for table %s. Synchronous planning does not retain
plan IDs.",
+ planId, tableIdentifier);
+ }
+
+ @Override
+ public void cancelPlanning(
+ IcebergRequestContext context, TableIdentifier tableIdentifier, String
planId) {
+ // Validate the table exists first (consistent with planTableScan behavior)
+ icebergCatalogWrapperManager
Review Comment:
Same as above: this is the leaf implementation of the dispatcher interface
and the only layer that can validate the table exists, so it needs to be
implemented here. See the reply on `fetchPlanningResult` for the full reasoning.
--
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]
