pvary commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2852017353
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,38 @@ private Supplier<BaseTable> createTableSupplier(
}
private RESTTable restTableForScanPlanning(
- TableOperations ops, TableIdentifier finalIdentifier, RESTClient
restClient) {
- // server supports remote planning endpoint and server / client wants to
do server side planning
- if (endpoints.contains(Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN) &&
restScanPlanningEnabled) {
+ TableOperations ops,
+ TableIdentifier finalIdentifier,
+ RESTClient restClient,
+ Map<String, String> tableConf) {
+ // Get client-side and server-side scan planning modes
+ String clientModeConfig =
properties().get(RESTCatalogProperties.SCAN_PLANNING_MODE);
+ String serverModeConfig =
tableConf.get(RESTCatalogProperties.SCAN_PLANNING_MODE);
+
+ // Validate that client and server configs don't conflict
Review Comment:
I don’t have a strong opinion for how we consolidate the server- and
client‑side configs, other than that we should try to be consistent with how
other configuration behaviors are defined.
My main concern is the following part of the spec:
```
- `client` (default): Clients MUST use client-side scan planning
```
What exactly is the reason for calling this the “default”?
Does this mean that if the server sends no `scan-planning-mode` at all, the
client must treat it as `client`? If so, that implies the client should fail
whenever it attempts server‑side planning but the server didn’t return a
`scan-planning-mode`.
So we need to align either the spec or the implementation.
If we want to preserve the current code behavior, but avoid specifying
semantics for the case where the server sends nothing, then we should remove
the word “default,” for example:
```
- `scan-planning-mode`: Controls scan planning behavior for table
operations. Valid values:
- `client`: Clients MUST use client-side scan planning
- `server`: Clients MUST use server-side scan planning
```
Alternatively, if we do want to retain “default,” then we need to update the
implementation so that an absent server value is treated exactly the same as if
the server had explicitly returned `client`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
