pvary commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2845713981
##########
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 really understand the motivation for client-side configuration here.
Based on the specification:
```
- `scan-planning-mode`: Controls scan planning behavior for table
operations. Valid values:
- `client` (default): Clients MUST use client-side scan planning
- `server`: Clients MUST use server-side scan planning
```
my interpretation is that the server fully determines the scan planning
mode. If it specifies `server`, then server-side planning MUST be used; if it
specifies `client`, **or does not specify anything**, then client-side planning
MUST be used.
In this case, don’t really understand the motivation for having a
client‑side configuration for scan planning.
The server provided scan-planning-mode controls how scan planning is
performed for table operations. In this case, there is no remaining decision or
configuration surface on the client side. If the client specifies a different
value than the server, we simply fail (do we need a config just for failing?).
The only scenario in which client‑side specification could meaningful is
when the server does not explicitly specify a planning mode but does support
server‑side planning. In that case, the client may be able to choose between
client‑side and server‑side planning. This, however, is not what the current
spec appears to describe.
If we want to support this behavior, it should be stated explicitly in the
specification: the client may decide only when the server has not specified a
planning mode.
For example, the spec could say something like:
```
- `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
- If not specified, then the client is free to choose, provided that
server-side planning is available
```
Is this the expected behavior?
--
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]
