laserninja commented on code in PR #11634:
URL: https://github.com/apache/gravitino/pull/11634#discussion_r3788454321
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -181,6 +181,20 @@ public LoadCredentialsResponse getTableCredentials(
}
}
+ /**
+ * Whether this catalog wrapper supports server-side scan planning.
+ *
+ * <p>Gravitino implements scan planning locally on top of loaded table
metadata. For non-REST
+ * backends (Hive, JDBC, Memory, Custom) this always works. For REST
backends the upstream catalog
+ * owns scan planning; advertising the endpoint here could be misleading
when the upstream does
+ * not support it, so we do not advertise it.
+ *
+ * @return {@code true} if the scan-plan endpoint should be advertised
+ */
+ public boolean supportsScanPlanOperations() {
Review Comment:
yes we do now, and this gating is stale as a result.
FederatedCatalogWrapper.planTableScan delegates to the remote catalog (POST
{table}/plan, forwarding X-Iceberg-Access-Delegation for vended credentials and
rewriting the credential refresh endpoints back to this IRC). That landed in
[#10800](https://github.com/apache/gravitino/pull/10800) on Jul 13, a month
after this PR was opened, and only reached this branch when I merged main
yesterday.
Since FederatedCatalogWrapper extends CatalogWrapperForREST and doesn't
override supportsScanPlanOperations(), the inherited !isRESTCatalog() returns
false for exactly the catalogs that now support scan planning. So this would
hide V1_SUBMIT_TABLE_SCAN_PLAN from /v1/config for REST backends that can
actually serve it. The javadoc rationale is wrong too: we don't leave scan
planning to the upstream, we proxy to it.
Two ways forward, and I'd like your preference:
Drop the scan-plan gating. Correct today, since every backend supports scan
planning. But it leaves
[#11584](https://github.com/apache/gravitino/issues/11584) with nothing to gate
(view gating already existed before this PR), so this would effectively be
closed as obsolete.
Have FederatedCatalogWrapper override supportsScanPlanOperations() to report
whether the remote catalog advertises V1_SUBMIT_TABLE_SCAN_PLAN in its own
/v1/config. That keeps the capability-derived intent of
[#11584](https://github.com/apache/gravitino/issues/11584) and genuinely fixes
misleading advertisement for the federation case. Iceberg 1.11's RESTCatalog
exposes no accessor for the remote endpoint set, so the wrapper would need its
own cached /v1/config fetch; it already builds an HTTP client for the
credentials and plan paths.
I lean toward 2, but happy to go with 1 and close this out if you don't
think the capability gating justifies the extra remote fetch.
--
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]
