amogh-jahagirdar commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2673131242
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -37,12 +37,107 @@ private RESTCatalogProperties() {}
public static final String NAMESPACE_SEPARATOR = "namespace-separator";
- // Enable planning on the REST server side
- public static final String REST_SCAN_PLANNING_ENABLED =
"rest-scan-planning-enabled";
- public static final boolean REST_SCAN_PLANNING_ENABLED_DEFAULT = false;
+ // Configure scan planning mode
+ // Can be set by server in LoadTableResponse.config() or by client in
catalog properties
+ // Negotiation rules: ONLY beats PREFERRED, both PREFERRED = client wins
+ // Default when neither client nor server provides: client-preferred
+ public static final String SCAN_PLANNING_MODE = "scan-planning-mode";
+ public static final String SCAN_PLANNING_MODE_DEFAULT =
+ ScanPlanningMode.CLIENT_PREFERRED.modeName();
public enum SnapshotMode {
ALL,
REFS
}
+
+ /**
+ * Enum to represent scan planning mode configuration.
+ *
+ * <p>Can be configured by:
+ *
+ * <ul>
+ * <li>Server: Returned in LoadTableResponse.config() to advertise server
preference/requirement
+ * <li>Client: Set in catalog properties to set client
preference/requirement
+ * </ul>
+ *
+ * <p>When both client and server configure this property, the values are
negotiated:
+ *
+ * <p>Values:
+ *
+ * <ul>
+ * <li>CLIENT_ONLY - MUST use client-side planning. Fails if paired with
CATALOG_ONLY from other
Review Comment:
I'm still a bit skeptical that we even need the notion of preferences, e.g.
client-preferred, catalog-referred. it''s plausible that servers could have
more insights to give a more intelligent preference but it feels over
complicated compared to just having a "clients-choice" or something instead of
2 preferences. It simplifies the decision matrix logic below, and clients can
then use their own heuristics.
I think that's what it comes down to, is it better to have clients just make
intelligent choices when server side planning is available but not required, or
is it better for servers to indicate preferences.
##########
core/src/main/java/org/apache/iceberg/rest/ScanPlanningNegotiator.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTCatalogProperties.ScanPlanningMode;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Handles negotiation between client and server scan planning mode
configurations.
+ *
+ * <p>This class encapsulates the decision logic for determining whether to
use client-side or
+ * server-side scan planning based on:
+ *
+ * <ul>
+ * <li>Client mode (CLIENT_ONLY, CLIENT_PREFERRED, CATALOG_PREFERRED,
CATALOG_ONLY, or null)
+ * <li>Server mode (CLIENT_ONLY, CLIENT_PREFERRED, CATALOG_PREFERRED,
CATALOG_ONLY, or null)
+ * <li>Server capabilities (supports planning endpoint or not)
+ * </ul>
+ *
+ * <p><b>Negotiation Rules</b>
+ *
+ * <p>When both client and server configure scan planning mode, the values are
negotiated:
+ *
+ * <ul>
+ * <li><b>Incompatible hard requirements</b>: CLIENT_ONLY + CATALOG_ONLY =
FAIL
+ * <li><b>ONLY wins over PREFERRED</b>: When one side has "ONLY" and the
other has "PREFERRED",
+ * the ONLY requirement wins (inflexible beats flexible)
+ * <li><b>Both PREFERRED</b>: When both are PREFERRED (different types),
client config wins
+ * <li><b>Both same</b>: When both have the same value, use that planning
type
+ * <li><b>Only one configured</b>: Use the configured side (client or server)
+ * <li><b>Neither configured</b>: Use default (CLIENT_PREFERRED)
+ * </ul>
+ */
+public class ScanPlanningNegotiator {
Review Comment:
I'm not really sure we need a whole separate "negotiator" class, it feels a
bit over the top, and it's not really a negotiation imo. We're using a defined
set of rules to determine the planning mode. Have we considered just having a
static ScanPlanningMode#determinePlanningDecision
--
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]
