gaborkaszab commented on code in PR #13979:
URL: https://github.com/apache/iceberg/pull/13979#discussion_r2324518976
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -167,16 +167,28 @@ protected TableIdentifier buildIdentifier(Identifier
identifier) {
@Override
public Table loadTable(Identifier ident) throws NoSuchTableException {
+ return loadTableViaView(ident, Map.of());
+ }
+
+ @Override
+ public Table loadTableViaView(Identifier ident, Map<String, Object> context)
+ throws NoSuchTableException {
try {
- return load(ident);
+ return load(ident, context);
} catch (org.apache.iceberg.exceptions.NoSuchTableException e) {
throw new NoSuchTableException(ident);
}
}
@Override
public Table loadTable(Identifier ident, String version) throws
NoSuchTableException {
- Table table = loadTable(ident);
+ return loadTableViaView(ident, version, Map.of());
+ }
+
+ @Override
+ public Table loadTableViaView(Identifier ident, String version, Map<String,
Object> context)
Review Comment:
Same comment as in `core/`: Now a `loadTableViaView` function performs the
load table even not via view, that seems a bit weird.
##########
api/src/main/java/org/apache/iceberg/catalog/ContextAwareTableCatalog.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 that references a table */
+ String VIEW_IDENTIFIER_KEY = "view.identifier";
+
+ /**
+ * Load a table with additional context information.
+ *
+ * <p>Common context keys:
+ *
+ * <ul>
+ * <li>{@link #VIEW_IDENTIFIER_KEY}: The fully qualified identifier of the
view referencing this
+ * table
+ * </ul>
+ *
+ * @param identifier the table identifier to load
+ * @param context additional context information as key-value pairs
+ * @return the loaded table
+ * @throws NoSuchTableException if the table does not exist
+ */
+ Table loadTableViaView(TableIdentifier identifier, Map<String, Object>
context)
Review Comment:
Can we expect more context params to this function? Can't we have a simple
String parameter to hold the name of the view? Or Maybe a TableIdentifier?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -367,46 +368,67 @@ public boolean tableExists(SessionContext context,
TableIdentifier identifier) {
}
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);
+
+ // inject the context to query params
+ Map<String, String> queryParams = Maps.newHashMap(mode.params());
+
+ Object viewIdentifierObj =
viewContext.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
Review Comment:
nit: I find the `loadInternal` function short and compact to have a quick
understanding of the functionality. Would you mind moving this extra code to a
function like `referencedByToQueryParams` or something similar?
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java:
##########
@@ -106,6 +112,15 @@ public Table loadTable(TableIdentifier ident) {
return delegate.loadTable(ident);
}
+ @Override
+ public Table loadTableViaView(TableIdentifier identifier, Map<String,
Object> viewContext) {
+ if (delegate instanceof ContextAwareTableCatalog) {
Review Comment:
`delegate` is a `BaseSessionCatalog.AsCatalog` that now implements
`ContextAwareTableCatalog`. Is there a use-case or configuration where delegate
doesn't implement the new interface?
I see the concept in this class is rather to have a new member of type
`ContextAwareTableCatalog` that is converted from `delegate` and this new
function could directly call that. Similarly as `nsCatalog`.
Would this also work here? Do I miss something?
##########
api/src/main/java/org/apache/iceberg/catalog/ContextAwareTableCatalog.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 that references a table */
+ String VIEW_IDENTIFIER_KEY = "view.identifier";
+
+ /**
+ * Load a table with additional context information.
+ *
+ * <p>Common context keys:
+ *
+ * <ul>
+ * <li>{@link #VIEW_IDENTIFIER_KEY}: The fully qualified identifier of the
view referencing this
+ * table
+ * </ul>
+ *
+ * @param identifier the table identifier to load
+ * @param context additional context information as key-value pairs
+ * @return the loaded table
+ * @throws NoSuchTableException if the table does not exist
+ */
+ Table loadTableViaView(TableIdentifier identifier, Map<String, Object>
context)
+ throws NoSuchTableException;
Review Comment:
What if the referencing view doesn't exist? Shouldn't we check for the
referencing view and throw NoSuchViewException, or should we silently omit if
the view doesn't exist?
##########
api/src/main/java/org/apache/iceberg/catalog/ContextAwareTableCatalog.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 that references a table */
+ String VIEW_IDENTIFIER_KEY = "view.identifier";
Review Comment:
Do you think this can be similar to the query param that we adding to the
get request? More specifically, can this also be called something like
`referenced-by` pr `referenced-by-view`? That way the name of the input param
could be similar or same to the http header sent to the server.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -367,46 +368,67 @@ public boolean tableExists(SessionContext context,
TableIdentifier identifier) {
}
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);
+
+ // inject the context to query params
+ Map<String, String> queryParams = Maps.newHashMap(mode.params());
+
+ Object viewIdentifierObj =
viewContext.get(ContextAwareTableCatalog.VIEW_IDENTIFIER_KEY);
+ if (viewIdentifierObj instanceof String) {
+ String viewIdentifierString = (String) viewIdentifierObj;
+ String[] parts = viewIdentifierString.split("\\.");
+ if (parts.length >= 2) {
+ String[] namespaceParts = new String[parts.length - 1];
+ System.arraycopy(parts, 0, namespaceParts, 0, parts.length - 1);
+ String viewName = parts[parts.length - 1];
+ queryParams.put(
+ "referenced-by",
+ RESTUtil.encodeNamespace(Namespace.of(namespaceParts))
+ .concat(RESTUtil.encodeString("." + viewName)));
+ }
+ }
+
return client
.withAuthSession(contextualSession)
.get(
paths.table(identifier),
- mode.params(),
+ queryParams,
LoadTableResponse.class,
Map.of(),
ErrorHandlers.tableErrorHandler());
}
@Override
- public Table loadTable(SessionContext context, TableIdentifier identifier) {
+ public Table loadTableViaView(
Review Comment:
I find it a bit weird that now even the non-view context behavior is
implemented in a function called `loadTableViaView`. In case I wanted to check
how load table is implemented, I don't think I would start with a function
`loadTableViaView`.
Just an idea: Can we make this function and the original `loadTable` to call
a new private `loadTable` that takes care the internals?
##########
api/src/main/java/org/apache/iceberg/catalog/ContextAwareTableCatalog.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 that references a table */
Review Comment:
This is package private now, but do we expect external clients to also
leverage this functionality? Shouldn't we give such a constant to clients/users
as public, for instance in a way I do extract the configs
[here](https://github.com/apache/iceberg/pull/13991)?
--
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]
