amogh-jahagirdar commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2799607881
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -59,4 +59,45 @@ public enum SnapshotMode {
ALL,
REFS
}
+
+ /**
+ * Enum to represent scan planning mode.
+ *
+ * <p>Can be configured by server in LoadTableResponse.config() to control
scan planning behavior
+ * per table.
+ *
+ * <p>Values:
+ *
+ * <ul>
+ * <li>CLIENT (default) - Use client-side scan planning
+ * <li>CATALOG - Use server-side scan planning (if server supports it)
+ * </ul>
+ */
Review Comment:
I feel like we can do without the comment here on this enum. It's mostly
referring to things outside the scope of the enum itself like in which context
would a server set it and what's the "default" which shouldn't be relevant just
for an enum definition
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -59,4 +59,45 @@ public enum SnapshotMode {
ALL,
REFS
}
+
+ /**
+ * Enum to represent scan planning mode.
+ *
+ * <p>Can be configured by server in LoadTableResponse.config() to control
scan planning behavior
+ * per table.
+ *
+ * <p>Values:
+ *
+ * <ul>
+ * <li>CLIENT (default) - Use client-side scan planning
+ * <li>CATALOG - Use server-side scan planning (if server supports it)
+ * </ul>
+ */
+ public enum ScanPlanningMode {
+ CLIENT("client"),
+ CATALOG("catalog");
+
+ private final String modeName;
+
+ ScanPlanningMode(String modeName) {
+ this.modeName = modeName;
+ }
+
+ public String modeName() {
+ return modeName;
+ }
+
+ public static ScanPlanningMode fromString(String mode) {
+ if (mode == null) {
Review Comment:
I don't think the enum itself should make a decision on what the default is
if `mode` is null. I would expect the caller of that to handle that before
handing off to ScanPlanningMode.fromString
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -59,4 +59,45 @@ public enum SnapshotMode {
ALL,
REFS
}
+
+ /**
+ * Enum to represent scan planning mode.
+ *
+ * <p>Can be configured by server in LoadTableResponse.config() to control
scan planning behavior
+ * per table.
+ *
+ * <p>Values:
+ *
+ * <ul>
+ * <li>CLIENT (default) - Use client-side scan planning
+ * <li>CATALOG - Use server-side scan planning (if server supports it)
+ * </ul>
+ */
+ public enum ScanPlanningMode {
+ CLIENT("client"),
+ CATALOG("catalog");
+
+ private final String modeName;
+
+ ScanPlanningMode(String modeName) {
+ this.modeName = modeName;
+ }
+
+ public String modeName() {
+ return modeName;
+ }
+
+ public static ScanPlanningMode fromString(String mode) {
+ if (mode == null) {
+ return CLIENT;
+ }
+ for (ScanPlanningMode planningMode : values()) {
+ if (planningMode.modeName.equalsIgnoreCase(mode)) {
+ return planningMode;
+ }
+ }
Review Comment:
style nit: new lines after the if statement, for loop
##########
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:
We have a choice here. We can either hard fail on conflicting configs like
is done below or we can just let one override the other.
I _think_ the least surprising thing from a client perspective is to just
override using the client config in case of conflicts. On average I don't
expect people to be fiddling with this config. In the chance they do, they're
probably intentionally trying to set it. Even if there's some FGAC and creds
and all aren't returned and it fails, that's better to me.
I guess I could go either way here (as long as we log there's a conflict and
we're overriding using some mechanism), but I think any approach seems better
than just hard failing since that seems unneccessary.
--
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]
