amogh-jahagirdar commented on code in PR #15572:
URL: https://github.com/apache/iceberg/pull/15572#discussion_r2920280212
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -59,4 +60,26 @@ public enum SnapshotMode {
ALL,
REFS
}
+
+ public enum ScanPlanningMode {
+ CLIENT,
+ SERVER;
+
+ public String modeName() {
+ return name().toLowerCase(Locale.ROOT);
+ }
+
+ public static ScanPlanningMode fromString(String mode) {
+ for (ScanPlanningMode planningMode : values()) {
+ if (planningMode.modeName().equalsIgnoreCase(mode)) {
+ return planningMode;
+ }
+ }
+
+ throw new IllegalArgumentException(
+ String.format(
+ "Invalid scan planning mode: %s. Valid values are: %s, %s",
+ mode, CLIENT.modeName(), SERVER.modeName()));
+ }
Review Comment:
Minor:
```
throw new IllegalArgumentException(
String.format(
"Invalid scan planning mode: %s. Valid values are: %s",
mode, Arrays.stream(values())
.map(ScanPlanningMode::modeName)
.collect(Collectors.joining(", "))));
```
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -953,4 +1010,71 @@ public void serverSupportsPlanningButNotCancellation()
throws IOException {
// Verify no exception was thrown - cancelPlan returns false when endpoint
not supported
assertThat(cancelled).isFalse();
}
+
+ @Test
+ public void serverConfigTakesPrecedenceOnMismatch() {
+ // Client=SERVER, Server=CLIENT → server wins → effective=CLIENT → returns
BaseTable
+ CatalogWithAdapter catalogWithAdapter1 =
+ catalogWithModes(
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.ScanPlanningMode.CLIENT.modeName());
+ catalogWithAdapter1.catalog.createNamespace(NS);
+
+ Table table1 =
+ catalogWithAdapter1
+ .catalog
+ .buildTable(TableIdentifier.of(NS, "mismatch_test"), SCHEMA)
+ .create();
+
+ assertThat(table1).isNotInstanceOf(RESTTable.class);
+ assertThat(table1).isInstanceOf(BaseTable.class);
+
+ // Client=CLIENT, Server=SERVER → server wins → effective=SERVER → returns
RESTTable
Review Comment:
Same as above
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -953,4 +1010,71 @@ public void serverSupportsPlanningButNotCancellation()
throws IOException {
// Verify no exception was thrown - cancelPlan returns false when endpoint
not supported
assertThat(cancelled).isFalse();
}
+
+ @Test
+ public void serverConfigTakesPrecedenceOnMismatch() {
+ // Client=SERVER, Server=CLIENT → server wins → effective=CLIENT → returns
BaseTable
Review Comment:
Minor: Not a useful comment, it's clear from the test name that we desire
server config to take precedence and we shouldn't really have comments that
outline how internal implementation works (i.e. "return baseTable") because
that stuff can always change and these comments become dated. It's below the
abstraction we care about here.
##########
open-api/rest-catalog-open-api.py:
##########
@@ -1468,6 +1468,9 @@ class LoadTableResult(BaseModel):
## General Configurations
- `token`: Authorization bearer token to use for table requests if OAuth2
security is enabled
+ - `scan-planning-mode`: Communicates to clients the supported planning
mode. Clients should use this value to fail fast if the supported scanning mode
is not available on the client. Valid values:
+ - `client`: Clients MUST use client-side scan planning
Review Comment:
After the spec change is in, we'll rebase and this (and the below) will be
removed right?
--
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]
