Repository navigation
Spark: [3/N] Support sending referenced-by to all endpoints - #13979
singhpk234 wants to merge 10 commits into
Conversation
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for the PR @singhpk234 !
I took a look for my own benefit, and left some observations and questions meanwhile. Let me know what you think!
|
|
||
| @Override | ||
| public Table loadTableViaView(TableIdentifier identifier, Map<String, Object> viewContext) { | ||
| if (delegate instanceof ContextAwareTableCatalog) { |
There was a problem hiding this 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?
97296ab to
bcec3f2
Compare
a34c0f2 to
fb6e824
Compare
20d30b3 to
737fc50
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
f368355 to
57e3e30
Compare
gaborkaszab
left a comment
There was a problem hiding this comment.
I went through the api/ and core/ part for now (haven't checked Spark). Left some questions and nits. I also found some of my previous comments so I'm not sure this was ready for review.
Anyway, some test coverage in TestRESTCatalog would be great!
|
Hi all - the Iceberg REST spec was merged 😃 This PR is now the next step in getting this referenced-by feature into the different Iceberg clients, right? Looking forward to get it merged & released |
57e3e30 to
a300631
Compare
684da4f to
acc960c
Compare
2479b08 to
be411cc
Compare
be411cc to
bce1ced
Compare
|
|
||
| case u @ UnresolvedRelation(parts @ CatalogAndIdentifier(catalog, ident), _, _) => | ||
| ViewUtil | ||
| .loadView(catalog, ident) |
There was a problem hiding this comment.
I think the loadView problem is back. With the flag on, every table read now calls loadView first, then Spark loads the table, so a REST catalog gets an extra 404 for each table read.
Maybe we can keep the single loadRelation call from the earlier version, and just return u for V1Table / DelegatingTable? Something like:
catalog.loadRelation(ident) match {
case view: View if ViewUtil.isIcebergView(view) => createViewRelation(parts, catalog, ident, view)
case _: V1Table | _: DelegatingTable => u
case table: Table => SubqueryAlias(..., DataSourceV2Relation.create(table, ...))
case _ => u
}|
|
||
| val crossCatalogViews = viewChain.filter(parts => !parts.headOption.contains(targetCatalogName)) | ||
| if (crossCatalogViews.nonEmpty) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
This throws again in the latest commit, and crossCatalogViewAccessFailsWhenReferencedByIsEnabled is back too (the Oct 5 version kept only the same-catalog views instead). Is failing the intended behavior now?
There was a problem hiding this comment.
Yes it is, because if we don't throw then we are not giving catalog the complete context ...
| @@ -33,6 +33,8 @@ class IcebergSparkSessionExtensions extends (SparkSessionExtensions => Unit) { | |||
| extensions.injectParser { case (_, parser) => new IcebergSparkSqlExtensionsParser(parser) } | |||
|
|
|||
| // analyzer extensions | |||
| // Run before Spark's ResolveRelations so Iceberg can rewrite view children with load context. | |||
| extensions.injectHintResolutionRule { spark => ResolveViews(spark) } | |||
There was a problem hiding this comment.
This also runs when the flag is off, so it changes current behavior. With an Iceberg view and a temp view both named shadow_v:
DESCRIBE TABLE shadow_v
-- flag off, with this line: returns the Iceberg view's columns
-- flag off, without this line: returns the temp view's columnsThe hint batch runs before Spark resolves temp views, so the Iceberg view now wins. Could the hint-batch rule do nothing when the flag is off?
There was a problem hiding this comment.
planning to make this a new rule to get rid of this issue and this rule will only come into effect when the flag is on
| * version refresh before execution reloads tables by identifier only, using the relation's | ||
| * catalog. Only read operations are supported. | ||
| */ | ||
| private[analysis] case class LoadContextTableCatalog( |
There was a problem hiding this comment.
LoadContext doesn't define equals/hashCode, and every query creates a new one. So two plans for the same view never compare equal here. For example, with the flag on:
Dataset<Row> df1 = spark.sql("SELECT * FROM simple_view");
df1.cache();
df1.count();
spark.sql("SELECT * FROM simple_view"); // doesn't use the cache (it does with the flag off)Could we add equals/hashCode to LoadContext?
There was a problem hiding this comment.
should i test for this ? i think the existing equals / hashcode should be sufficient ... i am not sure if i fully understand the example though ... i though df.cache() is only used when used in context of df ... is spark.sql will be aware of the cache of df1 ?
a9a313c to
4693c3f
Compare
Resolve the chain of views a relation was reached through and pass it to the catalog as a LoadContext, so a REST catalog can forward it to the server. SparkSupportsLoadContext extends the loading contract with context-carrying overloads of loadTable and loadView, including the time-travel forms, and SparkCatalog and SparkSessionCatalog implement it. ResolveViews records the chain while resolving nested views, with UnResolvedRelationFromView carrying it through analysis. Cross-catalog view references fail rather than silently dropping the chain. Gated behind spark.sql.iceberg.referenced-by-enabled, off by default.
Generated-by: Codex
Generated-by: Codex
TestSparkCatalog only overrode loadTable(Identifier), so once the read path started resolving through loadRelation -> loadTable(Identifier, LoadContext), the double was no longer consulted and lookups fell through to the real catalog. Override the contextual variant to delegate to the existing one. The double ignores the context; TestReferencedByViewChain covers the referenced-by behaviour against real catalogs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restore the Spark 4.1-style analyzer path for referenced-by propagation in Spark 4.2. ResolveViews now expands Iceberg views, carries the view chain through UnResolvedRelationFromView, and loads nested relations with an explicit LoadContext instead of using SQLConf-backed view metadata. Generated-by: Codex
Preserve existing cross-catalog view behavior by dropping catalog-relative referenced-by entries that cannot be represented for the target catalog. Bypass CachingCatalog for non-empty LoadContext loads so referenced-by reaches catalogs such as REST, and add regression coverage for cache-enabled Spark reads, time-travel views, and cross-catalog view references. Generated-by: Codex
Add a default-off Spark SQL flag for referenced-by view-chain propagation. When disabled, Spark keeps its native view resolution behavior, including cross-catalog views. When enabled, Iceberg uses the explicit analyzer path and rejects cross-catalog chains because LoadContext carries catalog-relative identifiers. Generated-by: Codex
Build the referenced-by view chain from the catalog and identifier Spark resolved when loading the view. Keep view metadata current catalog and namespace only for resolving unqualified references inside the view SQL. Generated-by: Codex
Keep the view chain on Spark's pre-execution table refresh by binding it to the catalog of relations resolved through a view, so a table read through a view is never reloaded as direct access. Convert view time travel with Spark's TimeTravelSpec so string and integer timestamps work and pinned snapshots are not refreshed. Leave V1 and delegating tables read through a view to Spark, and alias view-resolved tables so table-qualified columns resolve. Remove loadRelation overrides that duplicate BaseCatalog.
Move referenced-by view expansion out of ResolveViews into ResolveReferencedByViews, which does nothing when referenced-by is disabled. ResolveViews is back to its main behavior. With referenced-by enabled, BaseCatalog.loadRelation returns tables only, so Spark resolves tables itself and leaves Iceberg views unresolved for the new rule. This removes the hint-batch registration, which Spark's Hive session state ignores, and keeps a table read to a single load. Expanded views are wrapped in Spark's View node so cycle detection and cache invalidation keep working, and parse, streaming and not-found errors match Spark's. LoadContext gains equals and hashCode so cached plans over a view are reused by later queries. SparkSessionCatalog fails a load with a referenced-by chain when its Iceberg catalog cannot pass the context, instead of dropping it. Generated-by: Claude Code
4693c3f to
e8b01f5
Compare
About the change
This provides a reference implementation for passing the view name that table is referenced in as part of which the loadTable call is being made.
Details on the spec change proposal and motivation here