huaxingao commented on code in PR #13979:
URL: https://github.com/apache/iceberg/pull/13979#discussion_r4043084915
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -179,17 +181,49 @@ protected TableIdentifier buildIdentifier(Identifier
identifier) {
@Override
public Table loadTable(Identifier ident) throws NoSuchTableException {
- return load(ident, null /* no time travel */);
+ return loadTable(ident, LoadContext.empty());
+ }
+
+ @Override
+ public Table loadTable(Identifier ident, LoadContext context) throws
NoSuchTableException {
+ return load(ident, null /* no time travel */, context);
}
@Override
public Table loadTable(Identifier ident, String version) throws
NoSuchTableException {
- return load(ident, TimeTravel.version(version));
+ return loadTable(ident, version, LoadContext.empty());
Review Comment:
For a view over time travel like
```
SELECT id FROM test_table VERSION AS OF <snapshot>
```
does the catalog still get the view in `referencedBy`? loadRelation below
builds the context with ViewUtil.loadContext(catalogName), but this passes
LoadContext.empty(), so it looks like the view name is lost for time-travel
reads.
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -927,13 +967,14 @@ private static void checkNotPathIdentifier(Identifier
identifier, String method)
}
}
- private Table load(Identifier ident, TimeTravel timeTravel) throws
NoSuchTableException {
+ private Table load(Identifier ident, TimeTravel timeTravel, LoadContext
context)
+ throws NoSuchTableException {
if (isPathIdentifier(ident)) {
return loadPath((PathIdentifier) ident, timeTravel);
}
try {
- org.apache.iceberg.Table table =
icebergCatalog.loadTable(buildIdentifier(ident));
+ org.apache.iceberg.Table table =
icebergCatalog.loadTable(buildIdentifier(ident), context);
Review Comment:
`icebergCatalog` here is a `CachingCatalog` when caching is enabled, which
is the default. `CachingCatalog` doesn't override `loadTable(TableIdentifier,
LoadContext)`, and the default method ignores the context, so the chain never
reaches the real catalog and a REST catalog never sends `referenced-by`.
I think if you set `cache-enabled` back to true in
`TestReferencedByViewChain`, the test would fail there.
##########
spark/v4.2/spark/src/main/scala/org/apache/spark/sql/catalyst/analysis/ViewUtil.scala:
##########
@@ -73,6 +84,68 @@ object ViewUtil {
None
}
+ def loadContext(targetCatalogName: String): LoadContext = {
+ LoadContext
+ .builder()
+ .referencedBy(buildReferencedByChain(currentViewChain,
targetCatalogName))
+ .build()
+ }
+
+ def withReferencedByContext(view: View, catalogName: String, ident:
Identifier): View = {
+ if (view == null || view.queryText() == null || view.columns() == null) {
+ view
+ } else {
+ val viewChain = currentViewChain :+ qualifiedView(catalogName, ident)
+ val sqlConfigs =
+ Option(view.sqlConfigs()).map(_.asScala.toMap).getOrElse(Map.empty) +
+ (REFERENCED_BY_CONTEXT -> encodeViewChain(viewChain))
+
+ SparkView
+ .applyOptionalFields(
+ new View.Builder()
+ .withColumns(view.columns())
+ .withProperties(view.properties())
+ .withQueryText(view.queryText())
+ .withCurrentCatalog(view.currentCatalog())
+ .withCurrentNamespace(view.currentNamespace())
+ .withSqlConfigs(sqlConfigs.asJava)
+ .withQueryColumnNames(view.queryColumnNames()),
+ view.schemaMode(),
+ sqlConfigs.asJava,
+ view.viewDependencies())
+ .build()
+ }
+ }
+
+ /**
+ * Build the referenced-by view chain from fully qualified view identifier
parts.
+ * Entries must belong to the same catalog as the loaded target.
+ */
+ def buildReferencedByChain(
+ viewChain: Seq[Seq[String]],
+ targetCatalogName: String): java.util.List[TableIdentifier] = {
+ viewChain.foreach { parts =>
+ require(
+ parts.size >= 3,
+ s"View chain entry must be fully qualified [catalog, namespace...,
name], got: " +
+ parts.mkString("."))
+ }
+
+ val crossCatalogViews = viewChain.filter(parts =>
!parts.headOption.contains(targetCatalogName))
+ if (crossCatalogViews.nonEmpty) {
+ throw new IllegalStateException(
Review Comment:
It's legal today for a view in one catalog to read a table in another
catalog. For example:
```
createView("cross_view", "SELECT id FROM
other_catalog.default.other_table");
spark.sql("SELECT * FROM cross_view").collectAsList();
```
With this change it throws.
--
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]
