stevenzwu commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2815053796
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,40 @@ 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
+ // Only validate if BOTH are explicitly set (not null)
+ if (clientModeConfig != null && serverModeConfig != null) {
+ RESTCatalogProperties.ScanPlanningMode clientMode =
+ RESTCatalogProperties.ScanPlanningMode.fromString(clientModeConfig);
+ RESTCatalogProperties.ScanPlanningMode serverMode =
+ RESTCatalogProperties.ScanPlanningMode.fromString(serverModeConfig);
+
+ if (clientMode != serverMode) {
Review Comment:
nit: use `Preconditions.checkState`
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,40 @@ 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);
Review Comment:
it might be more clear if these two variables are called
`planningModeClientConfig` and `planningModeServerConfig`. this is even more
true if we rename the `catalog` mode to `server` mode
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -81,22 +84,25 @@ public <T extends RESTResponse> T execute(
Class<T> responseType,
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
- if (ResourcePaths.config().equals(request.path())) {
+ Object body = roundTripSerialize(request.body(), "request");
+ HTTPRequest req =
ImmutableHTTPRequest.builder().from(request).body(body).build();
+ T response = super.execute(req, responseType, errorHandler,
responseHeaders);
+ response = roundTripSerialize(response, "response");
+
+ // Add scan planning mode to table config for LoadTableResponse
+ if (response instanceof LoadTableResponse) {
+ LoadTableResponse loadResponse = (LoadTableResponse) response;
return castResponse(
responseType,
- ConfigResponse.builder()
- .withEndpoints(
- Arrays.stream(Route.values())
- .map(r -> Endpoint.create(r.method().name(),
r.resourcePath()))
- .collect(Collectors.toList()))
- .withOverrides(
-
ImmutableMap.of(RESTCatalogProperties.REST_SCAN_PLANNING_ENABLED, "true"))
+ LoadTableResponse.builder()
+ .withTableMetadata(loadResponse.tableMetadata())
+ .addAllConfig(loadResponse.config())
+ .addConfig(RESTCatalogProperties.SCAN_PLANNING_MODE,
"catalog")
Review Comment:
nit: use enum's `modeName`
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -81,22 +84,25 @@ public <T extends RESTResponse> T execute(
Class<T> responseType,
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
- if (ResourcePaths.config().equals(request.path())) {
+ Object body = roundTripSerialize(request.body(), "request");
Review Comment:
curious why we switched the order for the roundTripSerialize
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -106,6 +112,29 @@ protected String catalogName() {
return "prod-with-scan-planning";
}
+ @BeforeEach
+ public void setupCatalogWithScanPlanning() throws Exception {
+ // Reinitialize the catalog with scan planning mode set on client side
+ // This matches what the server returns in LoadTableResponse
+ restCatalog.close();
+ restCatalog =
+ new RESTCatalog(
+ SessionCatalog.SessionContext.createEmpty(),
+ (config) ->
+ HTTPClient.builder(config)
+ .uri(config.get(CatalogProperties.URI))
+ .withHeaders(RESTUtil.configHeaders(config))
+ .build());
+ restCatalog.setConf(new org.apache.hadoop.conf.Configuration());
+ restCatalog.initialize(
+ catalogName(),
+ ImmutableMap.<String, String>builder()
+ .put(CatalogProperties.URI, httpServer.getURI().toString())
+ .put(CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.inmemory.InMemoryFileIO")
+ .put(RESTCatalogProperties.SCAN_PLANNING_MODE, "catalog")
Review Comment:
nit: use enum modeName
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -805,6 +834,71 @@ private static class CatalogWithAdapter {
}
}
+ /**
+ * Helper method to create a catalog with custom scan planning mode
configuration for testing
+ * client-server mode validation.
+ *
+ * @param clientMode The scan planning mode the client requests (null for
default)
+ * @param serverMode The scan planning mode the server returns (null for not
set)
+ * @return CatalogWithAdapter for testing
+ */
+ private CatalogWithAdapter catalogWithScanPlanningModes(String clientMode,
String serverMode) {
Review Comment:
similar comment for `clientConfig` and `serverConfig`
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3468,6 +3436,9 @@ components:
## General Configurations
- `token`: Authorization bearer token to use for table requests if
OAuth2 security is enabled
+ - `scan-planning-mode`: Controls scan planning behavior for table
operations. Valid values:
+ - `client` (default): Clients MUST use client-side scan planning
+ - `catalog`: Clients MUST use server-side scan planning if the
server supports it, otherwise MUST fall back to client-side planning
Review Comment:
is `server` better here, which might pair better with `client`? We also call
the plan endpoint `server-side planning.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,40 @@ 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
+ // Only validate if BOTH are explicitly set (not null)
+ if (clientModeConfig != null && serverModeConfig != null) {
+ RESTCatalogProperties.ScanPlanningMode clientMode =
Review Comment:
similarly maybe call them `clientConfig` and `serverConfig`.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -840,7 +934,22 @@ public <T extends RESTResponse> T execute(
return castResponse(
responseType,
ConfigResponse.builder().withEndpoints(endpoints).build());
}
- return super.execute(request, responseType, errorHandler,
responseHeaders);
+ T response = super.execute(request, responseType,
errorHandler, responseHeaders);
+
+ // Add scan planning mode to table config for LoadTableResponse
+ if (response instanceof LoadTableResponse) {
+ LoadTableResponse loadResponse = (LoadTableResponse)
response;
+ return castResponse(
+ responseType,
+ LoadTableResponse.builder()
+ .withTableMetadata(loadResponse.tableMetadata())
+ .addAllConfig(loadResponse.config())
+ .addConfig(RESTCatalogProperties.SCAN_PLANNING_MODE,
"catalog")
Review Comment:
similar comment on using enum modeName
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,40 @@ 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
+ // Only validate if BOTH are explicitly set (not null)
+ if (clientModeConfig != null && serverModeConfig != null) {
+ RESTCatalogProperties.ScanPlanningMode clientMode =
+ RESTCatalogProperties.ScanPlanningMode.fromString(clientModeConfig);
+ RESTCatalogProperties.ScanPlanningMode serverMode =
+ RESTCatalogProperties.ScanPlanningMode.fromString(serverModeConfig);
+
+ if (clientMode != serverMode) {
Review Comment:
also why do we need to convert to enum for this mismatch check? can't we
compare the string directly?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -805,6 +834,71 @@ private static class CatalogWithAdapter {
}
}
+ /**
+ * Helper method to create a catalog with custom scan planning mode
configuration for testing
+ * client-server mode validation.
+ *
+ * @param clientMode The scan planning mode the client requests (null for
default)
+ * @param serverMode The scan planning mode the server returns (null for not
set)
+ * @return CatalogWithAdapter for testing
+ */
+ private CatalogWithAdapter catalogWithScanPlanningModes(String clientMode,
String serverMode) {
+ RESTCatalogAdapter adapter =
Review Comment:
this part seems very similar to the `createAdapterForServer` private method
near the top of this file. are they the same?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -805,6 +834,71 @@ private static class CatalogWithAdapter {
}
}
+ /**
+ * Helper method to create a catalog with custom scan planning mode
configuration for testing
+ * client-server mode validation.
+ *
+ * @param clientMode The scan planning mode the client requests (null for
default)
+ * @param serverMode The scan planning mode the server returns (null for not
set)
+ * @return CatalogWithAdapter for testing
+ */
+ private CatalogWithAdapter catalogWithScanPlanningModes(String clientMode,
String serverMode) {
+ RESTCatalogAdapter adapter =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ public <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders) {
+ if (ResourcePaths.config().equals(request.path())) {
+ return castResponse(
+ responseType,
+ ConfigResponse.builder()
+ .withEndpoints(
+ Arrays.stream(Route.values())
+ .map(r -> Endpoint.create(r.method().name(),
r.resourcePath()))
+ .collect(Collectors.toList()))
+ .build());
+ }
+ Object body = roundTripSerialize(request.body(), "request");
+ HTTPRequest req =
ImmutableHTTPRequest.builder().from(request).body(body).build();
+ T response = super.execute(req, responseType, errorHandler,
responseHeaders);
+ response = roundTripSerialize(response, "response");
+
+ // Add server's scan planning mode to LoadTableResponse if
specified
+ if (response instanceof LoadTableResponse && serverMode !=
null) {
+ LoadTableResponse loadResponse = (LoadTableResponse)
response;
+ return castResponse(
+ responseType,
+ LoadTableResponse.builder()
+ .withTableMetadata(loadResponse.tableMetadata())
+ .addAllConfig(loadResponse.config())
+ .addConfig(RESTCatalogProperties.SCAN_PLANNING_MODE,
serverMode)
+ .addAllCredentials(loadResponse.credentials())
+ .build());
+ }
+
+ return response;
+ }
+ });
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config)
-> adapter);
+
+ ImmutableMap.Builder<String, String> configBuilder =
Review Comment:
nit: call it `clientConfigBuilder`
--
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]
