singhpk234 commented on code in PR #15903:
URL: https://github.com/apache/iceberg/pull/15903#discussion_r3328023234
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1576,4 +1577,84 @@ public void
clientRequestsClientAndServerReturnsNothing() {
assertThat(table).isNotInstanceOf(RESTTable.class);
assertThat(table).isInstanceOf(BaseTable.class);
}
+
+ @Test
+ public void defaultPlanningModeWhenNoneSpecified() {
+ CatalogWithAdapter catalogWithAdapter = catalogWithModes(null, null);
+ catalogWithAdapter.catalog.createNamespace(NS);
+
+ Table table =
+ catalogWithAdapter
+ .catalog
+ .buildTable(TableIdentifier.of(NS, "default_mode_test"), SCHEMA)
+ .create();
+
+
assertThat(table).isNotInstanceOf(RESTTable.class).isInstanceOf(BaseTable.class);
+ }
+
+ @Test
+ public void invalidPlanningModeConfiguredForClient() {
+ assertThatThrownBy(
+ () ->
+ catalogWithModes(
+ "invalid_mode",
RESTCatalogProperties.ScanPlanningMode.CLIENT.modeName()))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid scan planning mode: invalid_mode");
+ }
+
+ @Test
+ public void invalidPlanningModeConfiguredForServer() {
+ CatalogWithAdapter catalogWithAdapter =
+
catalogWithModes(RESTCatalogProperties.ScanPlanningMode.CLIENT.modeName(),
"invalid_mode");
+ catalogWithAdapter.catalog.createNamespace(NS);
+
+ assertThatThrownBy(
+ () ->
+ catalogWithAdapter
+ .catalog
+ .buildTable(TableIdentifier.of(NS,
"invalid_server_mode_test"), SCHEMA)
+ .create())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid scan planning mode: invalid_mode");
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {"client", "CLIENT", "Client", "cLiEnT", "server", "SERVER",
"Server", "sErVeR"})
+ public void planningModeWithDifferentCasesOnClient(String planningMode) {
+ CatalogWithAdapter catalogWithAdapter = catalogWithModes(planningMode,
null);
+ catalogWithAdapter.catalog.createNamespace(NS);
+
+ Table table =
+ catalogWithAdapter
+ .catalog
+ .buildTable(TableIdentifier.of(NS, "client_case_test_" +
planningMode), SCHEMA)
+ .create();
+
+ if (planningMode.equalsIgnoreCase("client")) {
+
assertThat(table).isNotInstanceOf(RESTTable.class).isInstanceOf(BaseTable.class);
+ } else {
+ assertThat(table).isInstanceOf(RESTTable.class);
+ }
Review Comment:
This part is almost common, may be extract this to common private func ?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -282,6 +284,10 @@ public void initialize(String name, Map<String, String>
unresolved) {
RESTCatalogProperties.NAMESPACE_SEPARATOR,
RESTCatalogProperties.NAMESPACE_SEPARATOR_DEFAULT);
+ String scanPlanningModeConfig =
mergedProps.get(RESTCatalogProperties.SCAN_PLANNING_MODE);
+ this.scanPlanningMode =
+ scanPlanningModeConfig == null ? null :
ScanPlanningMode.fromString(scanPlanningModeConfig);
Review Comment:
This is loosing the meaning and is a bit hard to follow, earlier code vars
were specific what is client side and what is server side, should we say client
side scan planning mode ?
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -81,19 +79,11 @@ public String modeName() {
}
public static ScanPlanningMode fromString(String mode) {
- for (ScanPlanningMode planningMode : values()) {
- if (planningMode.modeName().equalsIgnoreCase(mode)) {
- return planningMode;
- }
+ try {
+ return ScanPlanningMode.valueOf(mode.toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String.format("Invalid scan
planning mode: %s", mode));
Review Comment:
This error message is not exactly same, we are not showing what the valid
values are ?
--
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]
