gaborkaszab commented on code in PR #13979:
URL: https://github.com/apache/iceberg/pull/13979#discussion_r2686022459
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -361,4 +361,9 @@ default boolean namespaceExists(SessionContext context,
Namespace namespace) {
return false;
}
}
+
+ default Table loadTableWithContext(
+ SessionContext sessionContext, TableIdentifier ident, Map<String,
Object> context) {
+ return loadTable(sessionContext, ident);
Review Comment:
Have you considered throwing UnsupportedOperationException instead of
silently ignore the context param?
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -159,5 +159,10 @@ public boolean removeProperties(Namespace namespace,
Set<String> removals) {
public boolean namespaceExists(Namespace namespace) {
return BaseSessionCatalog.this.namespaceExists(context, namespace);
}
+
+ @Override
+ public Table loadTable(TableIdentifier identifier, Map<String, Object>
loadingContext) {
+ return BaseSessionCatalog.this.loadTableWithContext(context, identifier,
loadingContext);
Review Comment:
nit: maybe consider renaming `context` to `sessionContext` to avoid
confusion with the new type of context?
##########
api/src/main/java/org/apache/iceberg/catalog/ContextAwareTableCatalog.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.catalog;
+
+import java.util.Map;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+
+/**
+ * Extension interface for Catalog that supports context-aware table loading.
This enables passing
+ * additional context (such as view information) when loading tables.
+ */
+public interface ContextAwareTableCatalog {
+
+ /**
+ * Context key for the view identifier(s) that reference the table being
loaded.
+ *
+ * <p>The value should be a List<TableIdentifier> representing the
view(s) that reference
+ * the table. For nested views (a view referencing another view which
references the table), the
+ * list should be ordered from outermost view to the view that directly
references the table.
+ *
+ * <p>This structured format keeps namespace parts and view names separate
throughout the catalog
+ * layer, only being serialized into the REST API format when making the
actual HTTP request
+ * according to the spec: namespace parts joined by namespace separator
(default 0x1F), followed
+ * by dot, followed by view name, with multiple views comma-separated.
+ */
+ String VIEW_IDENTIFIER_KEY = "view.referenced-by";
+
+ /**
+ * Load a table with additional context information.
+ *
+ * <p>Common context keys:
+ *
+ * <ul>
+ * <li>{@link #VIEW_IDENTIFIER_KEY}: A List<TableIdentifier> of
view(s) referencing this
Review Comment:
Is it necessary to escape the lt and gt chars?
"List<TableIdentifier>"
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
+ private Map<String, String> referencedByToQueryParam(
+ Map<String, String> params, Map<String, Object> context) {
+ if (context.isEmpty() ||
!context.containsKey(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY)) {
+ return params;
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap(params);
+ Object viewIdentifierObj =
context.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+
+ if (!(viewIdentifierObj instanceof List)) {
+ throw new IllegalStateException(
+ "Invalid view identifier in context, expected List<TableIdentifier>:
"
+ + viewIdentifierObj);
+ }
+
+ @SuppressWarnings("unchecked")
+ List<TableIdentifier> viewIdentifiers = (List<TableIdentifier>)
viewIdentifierObj;
+
+ if (viewIdentifiers.isEmpty()) {
+ return params;
+ }
+
+ // Format per REST API spec:
+ // - Namespace parts joined by namespace separator (0x1F, URL-encoded as
%1F)
+ // - Namespace and view name separated by dot (.)
+ // - Multiple views separated by comma (,)
+ // - Commas in view names must be percent-encoded as %2C
+ List<String> formattedViews =
Lists.newArrayListWithCapacity(viewIdentifiers.size());
+ for (TableIdentifier viewId : viewIdentifiers) {
+ // Encode namespace using RESTUtil which handles the namespace separator
+ String encodedNamespace = RESTUtil.encodeNamespace(viewId.namespace());
+
+ // Encode view name - commas must be encoded as %2C
+ String encodedViewName = RESTUtil.encodeString(viewId.name());
+
+ // Combine: encodedNamespace + "." + encodedViewName
+ // Note: RESTUtil.encodeString already encodes the dot if present in the
view name,
+ // but we add an unencoded dot as the separator
+ formattedViews.add(encodedNamespace + "." + encodedViewName);
+ }
+
+ // Join multiple views with comma
+ queryParams.put("referenced-by", String.join(",", formattedViews));
+
+ return queryParams;
+ }
+
@Override
- public Table loadTable(SessionContext context, TableIdentifier identifier) {
+ public Table loadTableWithContext(
Review Comment:
Maybe this had been discussed already, but can't this be simply `loadTable`?
The params will tell us anyway that this is with context.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
Review Comment:
nit: here at the calcite the reader could have the expression that the order
of `referencedByToQueryParam` and `snapshotModeToParam` is interchangeable, but
in practice it isn't. Maybe having a separate function to `paramsForLoadTable`
that produces both?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
+ private Map<String, String> referencedByToQueryParam(
+ Map<String, String> params, Map<String, Object> context) {
+ if (context.isEmpty() ||
!context.containsKey(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY)) {
+ return params;
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap(params);
+ Object viewIdentifierObj =
context.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+
+ if (!(viewIdentifierObj instanceof List)) {
+ throw new IllegalStateException(
+ "Invalid view identifier in context, expected List<TableIdentifier>:
"
+ + viewIdentifierObj);
+ }
+
+ @SuppressWarnings("unchecked")
+ List<TableIdentifier> viewIdentifiers = (List<TableIdentifier>)
viewIdentifierObj;
Review Comment:
Shouldn't we include a check into the above that not just check for List
type but for List<TableIdentifier>? Wouldn't this result in a
ClassCastException or something similar if we input List that contains anything
other than TableIdentifier?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
+ private Map<String, String> referencedByToQueryParam(
+ Map<String, String> params, Map<String, Object> context) {
+ if (context.isEmpty() ||
!context.containsKey(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY)) {
+ return params;
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap(params);
+ Object viewIdentifierObj =
context.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+
+ if (!(viewIdentifierObj instanceof List)) {
+ throw new IllegalStateException(
+ "Invalid view identifier in context, expected List<TableIdentifier>:
"
+ + viewIdentifierObj);
+ }
+
+ @SuppressWarnings("unchecked")
+ List<TableIdentifier> viewIdentifiers = (List<TableIdentifier>)
viewIdentifierObj;
+
+ if (viewIdentifiers.isEmpty()) {
+ return params;
+ }
+
+ // Format per REST API spec:
+ // - Namespace parts joined by namespace separator (0x1F, URL-encoded as
%1F)
+ // - Namespace and view name separated by dot (.)
+ // - Multiple views separated by comma (,)
+ // - Commas in view names must be percent-encoded as %2C
+ List<String> formattedViews =
Lists.newArrayListWithCapacity(viewIdentifiers.size());
+ for (TableIdentifier viewId : viewIdentifiers) {
+ // Encode namespace using RESTUtil which handles the namespace separator
Review Comment:
nit: I don't think these comments in the for loop are necessary. The code
seems pretty straightforward and the longer comment above the loop also helps
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
+ private Map<String, String> referencedByToQueryParam(
+ Map<String, String> params, Map<String, Object> context) {
+ if (context.isEmpty() ||
!context.containsKey(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY)) {
+ return params;
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap(params);
+ Object viewIdentifierObj =
context.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+
+ if (!(viewIdentifierObj instanceof List)) {
Review Comment:
nit: maybe `Preconditions.checkArgument(viewIdentifierObj instanceof List,
error msg)`
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -396,46 +397,96 @@ private static Map<String, String>
snapshotModeToParam(SnapshotMode mode) {
}
private LoadTableResponse loadInternal(
- SessionContext context, TableIdentifier identifier, SnapshotMode mode) {
+ SessionContext context,
+ TableIdentifier identifier,
+ SnapshotMode mode,
+ Map<String, Object> viewContext) {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- snapshotModeToParam(mode),
+ referencedByToQueryParam(snapshotModeToParam(mode), viewContext),
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
+ private Map<String, String> referencedByToQueryParam(
+ Map<String, String> params, Map<String, Object> context) {
+ if (context.isEmpty() ||
!context.containsKey(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY)) {
+ return params;
+ }
+
+ Map<String, String> queryParams = Maps.newHashMap(params);
+ Object viewIdentifierObj =
context.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+
+ if (!(viewIdentifierObj instanceof List)) {
+ throw new IllegalStateException(
+ "Invalid view identifier in context, expected List<TableIdentifier>:
"
+ + viewIdentifierObj);
+ }
+
+ @SuppressWarnings("unchecked")
+ List<TableIdentifier> viewIdentifiers = (List<TableIdentifier>)
viewIdentifierObj;
+
+ if (viewIdentifiers.isEmpty()) {
+ return params;
+ }
+
+ // Format per REST API spec:
+ // - Namespace parts joined by namespace separator (0x1F, URL-encoded as
%1F)
Review Comment:
separator is configurable now, the comment now is invalid
--
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]
