You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yyanyy (via GitHub)" <gi...@apache.org> on 2026/07/28 00:52:11 UTC
[PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
yyanyy opened a new pull request, #57585:
URL: https://github.com/apache/spark/pull/57585
### What changes were proposed in this pull request?
Follow-up to #57582 (SPARK-58389), which forwarded user options to the table read path
but not the `RelationCatalog` single-RPC path. This PR adds
`RelationCatalog.loadRelation(Identifier, CaseInsensitiveStringMap)` (default ignores the
options and delegates to `loadRelation(Identifier)`, so existing connectors are
unaffected) and forwards the user options to it from `RelationResolution`.
### Why are the changes needed?
For a `RelationCatalog` (a connector exposing both tables and views), `loadRelation` is
the primary plain-read path, and it currently drops the user options. This is the
single-RPC counterpart of #57582 and lets such connectors see the read options when
producing the relation.
### Does this PR introduce _any_ user-facing change?
No. New connector-API entry point with a behavior-preserving default; not yet released.
### How was this patch tested?
New test in `DataSourceV2OptionSuite` asserting a `spark.read.option(...)` value reaches
`loadRelation`. Regression: `DataSourceV2OptionSuite`, `DataSourceV2MetadataViewSuite`,
`MetricViewV2CatalogSuite`, `SupportsCatalogOptionsSuite` (76/76); `DataSourceV2SQLSuite`
(V1/V2 filter) and the v2 view command suites (572/572).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code Opus 4.8
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5364295716
> Question: do we actually have to support state options in RelationCatalog? I think state options today apply only to tables, meaning we can follow what we do today for time travel and write privileges. If we detect state options, don't attempt to use the optimized loading and just try TableCatalog.
Thanks for the review!
I think it's better to route the state options to `loadRelation` instead of directly assuming it's a table, for 2 reasons:
1. I think this will introduce a real behavior regression. Consider the below case
```
spark.read
.option("snapshot", "s1")
.table("catalog.namespace.some_view")
```
Despite that arguably this is not the "right way to run the query", today we can return the existing view, ignoring `snapshot` argument. Once we make this change and connector implementor decides to treat `snapshot` as a table state option, Spark only treat it as a table and does not consider the existing view, and the same query will start to fail after upgrading.
It may feel like time travel have a similar challenge/breaking behavior but I think it's not a precedent. Before RelationCatalog was introduced, time travel already called the table-specific loadTable(ident, version/timestamp) overloads and never tried to resolve a view; adding loadRelation preserved that existing table-only behavior. Connector-defined state options are different: they are opaque options added to an otherwise ordinary read that previously could resolve a view.
2. I think this adds additional burden to the implementor of `RelationCatalog`. One of its benefits is that an implementer can complete the loading behavior by only implementing the below 2 APIs:
```
loadRelation(ident)
loadRelation(ident, stateOptions) // when the catalog consumes state options
```
If we implement this proposal, a state-ware RelationCatalog implementer must additionally implement `loadTable(ident, TableContext, stateOptions)`. We could add a default implementation of that `loadTable` overload to delegates back to `loadRelation(ident, stateOptions)`, but with that we have effectively reconstructed the current design with additional indirection.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3724246261
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read, so a catalog can take them into account when producing the {@link Table} or
+ * {@link View}. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; override it to make use of the options.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
+ * @return a {@link Table} for tables, or a {@link View} for views
+ * @throws NoSuchTableException if neither a table nor a view exists at {@code ident}
+ * @since 4.2.0
Review Comment:
fixed, thanks for the review!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Use loadTable for RelationCatalog reads with table-state options [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3869689742
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -277,19 +277,21 @@ class RelationResolution(
None
}
- // For a `RelationCatalog` with no time-travel / write privileges, the single-RPC
- // `loadRelation` answers both "is there a table?" and "is there a view?" in one
- // call. Time-travel and write privileges apply to tables only, so for those the
- // lookup falls through to the table-only `loadTable` path below; views are not
- // reachable via the v2 fallback in those cases.
+ // For a `RelationCatalog` with no table-state options, time travel, or write
+ // privileges, the single-RPC `loadRelation` answers both "is there a table?" and "is
+ // there a view?" in one call. Table-state options, time travel, and write privileges
Review Comment:
**Nit:**
These are conditions of this lookup, not properties of the `RelationCatalog`; making the lookup the subject avoids implying that the catalog cannot support them.
```suggestion
// When this lookup through a `RelationCatalog` has no table-state options, time travel,
// or write privileges, the single-RPC `loadRelation` answers both "is there a table?"
// and "is there a view?" in one call. Table-state options, time travel, and write
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5357789676
Question: do we actually have to support state options in RelationCatalog? I think state options today apply only to tables, meaning we can follow what we do today for time travel and write privileges. If we detect state options, don't attempt to use the optimized loading and just try TableCatalog.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5227943701
> Re-checked through [`6242054e103`](https://github.com/apache/spark/commit/6242054e10370db6d8b4085ecbe6c855a0eed7e4) — findings 1, 3, 4, 5, 6, 8 resolved, 2 and 7 answered (4.3 backport settled with @aokolnychyi and now stated in the description; the `V2TableReference` reload is #57799), nothing regressed. I re-traced the three option-carrying load sites — `V2TableRefreshUtil.refresh`, `CacheManager.tryRefreshPlan`, and the `SupportsCatalogOptions` `load()` path — and all three now reach `loadRelation(ident, options)` through the new default; the time-travel / write-privilege branches still go to `TableCatalog.super` untouched. On your "more `loadTable` variants" thought: agreed, don't. Keeping the plain-read routing as a `RelationCatalog` default and leaving the two table-only cases to `TableCatalog`'s dispatch is the right split — a relation-level `context` would always be empty and would invite connectors to implement semantics Spark never asks for. Two things left, bo
th text rather than behavior.
>
> ### Blocking
> * **9.** _Description doesn't cover the `loadTable` dispatch override (new):_ "What changes were proposed" still describes only the new `loadRelation(Identifier, CaseInsensitiveStringMap)` overload plus the `RelationResolution` callsite, but the `loadTable(Identifier, TableContext, CaseInsensitiveStringMap)` default that re-routes every plain-read table load into `loadRelation(ident, options)` is the half that actually reaches refresh / recache / `SupportsCatalogOptions`, and it is not mentioned at all. Two knock-ons in the same text: "existing connectors are unaffected" no longer holds literally — a `RelationCatalog` that overrides `loadTable(Identifier)` stops seeing plain reads through it (a consistency win, since the resolver already bypassed it, but worth a sentence) — and "How was this patch tested?" still says one new test where there are now four (DataFrame forwarding, view over a V2 table, execution refresh, and the dispatch matrix).
>
> ### Minor
> * **10.** _Class javadoc now contradicts the new override's javadoc (new):_ the "Single-RPC perf entry points" list says the 1-arg `loadRelation` is the base "the `loadTable` / `loadView` defaults derive from", while `:212` says `loadTable(Identifier, TableContext, CaseInsensitiveStringMap)` derives from the options-aware overload. [inline: `sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:113`]
Thank you @peter-toth for the review! I have updated the PR description and the java doc per feedback.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5357580618
I'll need to think about it a bit.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3726718760
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryRelationCatalog.scala:
##########
@@ -47,6 +47,14 @@ class InMemoryRelationCatalog extends RelationCatalog with SupportsNamespaces {
Option(store.get(key)).getOrElse(throw new NoSuchTableException(ident))
}
+ private var _lastLoadRelationOptions: Option[CaseInsensitiveStringMap] = None
Review Comment:
**Finding 5.** This records only the last bag, while the sibling fixture #57582 touched records every call: `InMemoryTableCatalog.scala:58-63` keeps `_loadTableCalls: ArrayBuffer[(TableContext, CaseInsensitiveStringMap)]` and exposes `loadTableCalls` / `resetLoadTableCalls()` / `lastLoadTableOptions` on top of it. That accumulator is what lets `DataSourceV2OptionSuite` assert the things a last-call recorder cannot express -- "one load per distinct option bag" (`:568`), "identical bags load once" (`:610`), "analysis *and* refresh both forwarded" (`:542`) -- and the `RelationCatalog` equivalents of all three are missing here; finding 1 needs the third. Suggest mirroring the sibling (plus the `scala.collection.mutable` import):
```scala
private val _loadRelationCalls = mutable.ArrayBuffer.empty[CaseInsensitiveStringMap]
def loadRelationCalls: Seq[CaseInsensitiveStringMap] = _loadRelationCalls.toSeq
def resetLoadRelationCalls(): Unit = _loadRelationCalls.clear()
def lastLoadRelationOptions: Option[CaseInsensitiveStringMap] = _loadRelationCalls.lastOption
```
and putting it in the "Test-only accessors" section at `:227` next to `getStoredInfo` / `getStoredView`, rather than between `loadRelation` and the `----- TableCatalog -----` banner.
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:
##########
@@ -81,6 +81,23 @@ class DataSourceV2OptionSuite extends DatasourceV2SQLBase {
}
}
+ test("Propagate options to RelationCatalog.loadRelation on read") {
Review Comment:
**Finding 8.** Nit: no JIRA prefix, and the test sits between the `SPARK-36680` and `SPARK-50286` cases rather than with the option-forwarding group at `:462`-`:540` / the `SPARK-58389:` block at `:542`+ that it is the sequel to. Something like `test("SPARK-58392: options are forwarded to loadRelation - DataFrame API")`, placed next to `"options are forwarded to loadTable - DataFrame API"`, would read as the pair it is.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
Review Comment:
**Finding 3.** The class javadoc's "Single-RPC perf entry points" list (`RelationCatalog.java:109`) still describes `loadRelation(Identifier)` as "the resolver's per-identifier read path", which this PR makes false -- the resolver now calls the overload, and the 1-arg one is left serving the DDL lookup in `Analyzer.lookupTableOrView` (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:1275`) plus the derived `loadTable` / `loadView` / `tableExists` / `viewExists` defaults. The new overload is not in that list at all. Rough shape:
```
* <li>{@link #loadRelation(Identifier)} -- returns a {@link Table} for a table or a
* {@link View} for a view; callers discriminate via {@code instanceof}. Saves the
* {@code loadTable} -> {@code loadView} fallback on a cold cache. Used for lookups that
* carry no read options (DDL and misc commands), and as the base that the
* {@code loadTable} / {@code loadView} defaults derive from.</li>
* <li>{@link #loadRelation(Identifier, CaseInsensitiveStringMap)} -- the resolver's
* per-identifier read path: same contract, plus the user's read options.</li>
```
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; catalogs that want to receive the user options while
+ * reading a relation must override this method.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
Review Comment:
**Finding 4.** "all options passed to the read" is accurate about the bag's contents but not about when it arrives: the resolver routes here only when there is no time travel and no write privileges (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:267`). A `versionAsOf` read or an `INSERT INTO ... WITH (...)` target never reaches this method -- both go to `TableCatalog.loadTable(Identifier, TableContext, CaseInsensitiveStringMap)`. #57582 was explicit about the analogous contract ("An override replaces that dispatch and must honor `context` itself..."); without the same note here, an implementor who overrides only this method reasonably believes they have covered reads. Suggest adding:
```
* Spark calls this for a plain read only -- no time travel and no write privileges, both of
* which apply to tables only and route through
* {@link TableCatalog#loadTable(Identifier, TableContext, CaseInsensitiveStringMap)} instead.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:
##########
@@ -81,6 +81,23 @@ class DataSourceV2OptionSuite extends DatasourceV2SQLBase {
}
}
+ test("Propagate options to RelationCatalog.loadRelation on read") {
+ registerCatalog("testrelcat", classOf[InMemoryRelationCatalog])
+ val t1 = "testrelcat.ns1.ns2.table"
+ withTable(t1) {
+ sql(s"CREATE TABLE $t1 (id bigint, data string) USING parquet")
+
+ val relCatalog = catalog("testrelcat").asInstanceOf[InMemoryRelationCatalog]
+ assert(relCatalog.lastLoadRelationOptions.isEmpty)
+
+ spark.read.option("customOption", "customValue").table(t1)
+ .queryExecution.analyzed
+ val recorded = relCatalog.lastLoadRelationOptions
+ assert(recorded.isDefined)
+ assert(recorded.get.get("customOption") == "customValue")
Review Comment:
**Finding 6.** `loadRelation` is the one load method that can return a `View`, and this new overload is what a combined table+view catalog will use for both kinds -- but only the table branch is covered. A view case is cheap: `CREATE VIEW testrelcat.ns1.ns2.v AS SELECT 1 AS x` seeds one through `InMemoryRelationCatalog.createView` (the same way `DataSourceV2MetadataViewSuite.seedV2View` does against its own `RelationCatalog`), then `spark.read.option("customOption", "customValue").table("testrelcat.ns1.ns2.v")` and the same recorded-bag assertion. Worth having: a connector may well want to pick a view definition variant from the read options, and `RelationResolution.createRelation` takes a visibly different branch for `View` (`createDataSourceV1Scan(V1Table.toCatalogTable(...))`) than for `Table`.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; catalogs that want to receive the user options while
+ * reading a relation must override this method.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
+ * @return a {@link Table} for tables, or a {@link View} for views
+ * @throws NoSuchTableException if neither a table nor a view exists at {@code ident}
+ * @since 4.3.0
+ */
+ default Relation loadRelation(Identifier ident, CaseInsensitiveStringMap options)
Review Comment:
**Finding 1.** The options only reach a `RelationCatalog` from the resolver's plain-read branch. Every other load-with-options goes through `TableCatalog.loadTable(Identifier, TableContext, CaseInsensitiveStringMap)`, which `RelationCatalog` does not override -- so for an empty context its default dispatches to `loadTable(ident)` (`TableCatalog.java:238`), which `RelationCatalog` derives from `loadRelation(ident)` (`RelationCatalog.java:196`), and the options are gone. All three of these call it with a non-empty options bag and an empty context:
- `V2TableRefreshUtil.refresh` -> `CatalogV2Util.getTable(catalog, ident, options = r.options)` (`sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2TableRefreshUtil.scala:99`)
- `CacheManager.tryRefreshPlan` -> the same call (`sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:422`)
- the `SupportsCatalogOptions` `spark.read.format(...).option(...).load()` path (`sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Utils.scala:144`)
The refresh one is the one that bites: it reloads and then does `r.copy(table = currentTable)`, so a `Table` that analysis shaped from the options is silently replaced at execution time by the one loaded without them.
Measured with a `RelationCatalog` fixture that returns a real v2 `Table` and records both overloads ("bare" = 1-arg). For a single `spark.read.option("split-size", "5").table(t).collect()`:
relationLoads = [options:5, bare] // analysis forwards, the refresh does not
Adding this default turns it into `[options:5, options:5]`, with `DataSourceV2OptionSuite` (29 tests, including the whole `SPARK-58389:` block) still green:
```java
/**
* {@inheritDoc}
* <p>
* The default implementation derives from {@link #loadRelation(Identifier,
* CaseInsensitiveStringMap)} for a plain read, so the user options reach the relation-level
* entry point. Time-travel and write-privilege loads keep {@link TableCatalog}'s dispatch --
* both apply to tables only.
*/
@Override
default Table loadTable(
Identifier ident,
TableContext context,
CaseInsensitiveStringMap options) throws NoSuchTableException {
if (context.timeTravel().isPresent() || !context.writePrivileges().isEmpty()) {
return TableCatalog.super.loadTable(ident, context, options);
}
if (loadRelation(ident, options) instanceof Table t) {
return t;
}
throw new NoSuchTableException(ident);
}
```
Worth noting why the new test cannot see this: `InMemoryRelationCatalog` stores tables as `DelegatingTable`, which `RelationResolution.createRelation` routes down the v1 scan path, so the read never becomes a `DataSourceV2Relation` and never reaches the refresh at all. A fixture returning a plain v2 `Table` (plus the call accumulator from finding 5) is what a regression test needs -- the `RelationCatalog` analogue of `"SPARK-58389: execution refresh forwards options on a plain table read"`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3743283049
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -178,6 +206,28 @@ default Table loadTable(Identifier ident) throws NoSuchTableException {
throw new NoSuchTableException(ident);
Review Comment:
**Finding 11.** With the `loadTable(Identifier, TableContext, CaseInsensitiveStringMap)` default added below, `loadTable(Identifier)` is off the read path entirely for a `RelationCatalog`: analysis calls `loadRelation(ident, options)` (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:269`), and refresh / recache / the `SupportsCatalogOptions` `load()` path all arrive at `:225`, which derives from the same overload. What still lands here is write-privilege loads (`TableCatalog`'s `loadTable(ident, writePrivileges)` default falls through, `TableCatalog.java:238`), DDL and misc commands (`ShowTablesExtendedExec.scala:52`, `RenameTableExec.scala:53`, `TableCatalog#listTableSummaries` at `TableCatalog.java:135`) and the `V2TableReference` reload (`RelationResolution.scala:510`).
So the advice at `:198` — "Override only if a tables-only path is materially cheaper than the unified one" — now points at an optimization that never fires for the case it reads like it is for. The description states the consequence; the javadoc is where a connector author will look for it. Rough shape:
```java
* The default implementation derives from {@link #loadRelation}: a {@link View} is rejected as
* not-a-table; a {@link Table} is returned. Override only if a tables-only path is materially
* cheaper than the unified one -- note that reads do not reach this method: they go to
* {@link #loadRelation(Identifier, CaseInsensitiveStringMap)}, directly from the resolver or
* via {@link #loadTable(Identifier, TableContext, CaseInsensitiveStringMap)}. What remains
* here is write-privilege loads, DDL and misc lookups.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3855329314
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +139,34 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * the user-specified options that may affect table state.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives table-state options. The
Review Comment:
What worries me here is that we start passing the table state options as relation options, without thinking about view state options. Maybe, it is fine.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Use loadTable for RelationCatalog reads with table-state options [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #57585: [SPARK-58392][SQL] Use loadTable for RelationCatalog reads with table-state options
URL: https://github.com/apache/spark/pull/57585
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3741461076
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -105,10 +106,13 @@
* questions in two round trips each. {@code RelationCatalog} adds dedicated methods so a
* catalog can answer both in one round trip:
* <ul>
- * <li>{@link #loadRelation(Identifier)} -- the resolver's per-identifier read path. Returns a
- * {@link Table} for a table or a {@link View} for a view; callers discriminate via
- * {@code instanceof}. Saves the {@code loadTable} -> {@code loadView} fallback on a cold
- * cache.</li>
+ * <li>{@link #loadRelation(Identifier)} -- returns a {@link Table} for a table or a
+ * {@link View} for a view; callers discriminate via {@code instanceof}. Saves the
+ * {@code loadTable} -> {@code loadView} fallback on a cold cache. Used for lookups that
+ * carry no read options (DDL and miscellaneous commands), and as the base that the
+ * {@code loadTable} / {@code loadView} defaults derive from.</li>
Review Comment:
**Finding 10.** My round-1 wording predated the `loadTable` override you added, so it is now a hair off: `loadTable(Identifier, TableContext, CaseInsensitiveStringMap)` derives from the *options-aware* overload (`:212`, `:225`), not from this one. Only `loadTable(Identifier)` (`:203`) and `loadView` (`:241`) still derive from the 1-arg. Naming the overload keeps the two javadocs from disagreeing:
```suggestion
* {@code loadTable(Identifier)} / {@code loadView} defaults derive from.</li>
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3751884864
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -178,6 +206,28 @@ default Table loadTable(Identifier ident) throws NoSuchTableException {
throw new NoSuchTableException(ident);
Review Comment:
Addressed, thanks a lot Peter for the detailed review!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5210325820
> Thanks for the PR, @yyanyy!
>
> Follow-up to #57582: this adds `RelationCatalog.loadRelation(Identifier, CaseInsensitiveStringMap)` with a behavior-preserving default and forwards `finalOptions` to it from the resolver's plain-read branch, so a combined table+view catalog can see the read options at metadata-load time. The overload shape is right and the default keeps existing connectors untouched. The problem is that the wiring stops at the resolver: `RelationCatalog` doesn't route the new options-aware entry point into the `loadTable` side, so every _other_ options-aware load -- the execution-time refresh, recaching, and the `SupportsCatalogOptions` `format(...).load()` path -- still lands on the option-less `loadRelation(Identifier)`. I reproduced that against a fixture returning a real v2 `Table` and confirmed a small default override closes it (finding 1). The rest are docs, test-fixture and coverage points.
>
> ### Blocking
> * **1.** _Options reach `loadRelation` only from the resolver:_ `RelationCatalog` doesn't override `TableCatalog.loadTable(Identifier, TableContext, CaseInsensitiveStringMap)`, so with an empty context that default dispatches to `loadTable(ident)` -> `loadRelation(ident)` and the options are dropped on the refresh / recache / `SupportsCatalogOptions` paths. Measured `[options:5, bare]` for one `spark.read.option("split-size", "5").table(t).collect()`; a default override deriving from `loadRelation(ident, options)` makes it `[options:5, options:5]` with `DataSourceV2OptionSuite` still green. [inline: `sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:153`]
>
> ### Non-blocking
> * **2.** _`@since 4.3.0` needs re-checking:_ @uros-b's `@since` thread settled on 4.3.0 on 2026-07-28, when `branch-4.x` was 4.3.0. It was bumped to 4.4.0 on 2026-08-03 ([SPARK-58534](https://issues.apache.org/jira/browse/SPARK-58534)) and `branch-4.3` is cut and still `4.3.0-SNAPSHOT`, so 4.3.0 now holds only if this is also backported there. Worth stating the intent in the description, since otherwise 4.3.0 ships #57582's API without its `RelationCatalog` half. (Not opening a new thread -- this is a note on that one.)
> * **3.** _Class javadoc drift:_ the "Single-RPC perf entry points" list still calls `loadRelation(Identifier)` "the resolver's per-identifier read path", which this PR makes false, and the new overload isn't listed at all. [inline: `sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:138`]
> * **4.** _The new javadoc doesn't say when Spark calls it:_ the resolver routes here only for a plain read (no time travel, no write privileges); an implementor who overrides just this method reasonably believes they have covered reads. #57582 spelled out the analogous contract. [inline: `sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:148`]
> * **5.** _Recorder diverges from the fixture convention #57582 established:_ `InMemoryTableCatalog` accumulates every call (`loadTableCalls` / `resetLoadTableCalls()`); this records only the last bag, which is exactly what blocks the three `SPARK-58389:` style assertions -- including the one finding 1 needs. [inline: `sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryRelationCatalog.scala:50`]
> * **6.** _No view-side coverage:_ `loadRelation` is the one load method that can return a `View`, and the test only exercises the table branch. [inline: `sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:97`]
> * **7.** _`V2TableReference` reload still drops `ref.options` (follow-up, not this PR):_ `RelationResolution.loadRelation(ref)` keys its cache on `ref.options` (`:488`) and builds the relation with `ref.options` (`:517`), but reloads via `resolvedCatalog.loadTable(ref.identifier)` (`:510`) -- no options. Same shape as what [SPARK-58389](https://issues.apache.org/jira/browse/SPARK-58389) fixed elsewhere; suggest a separate JIRA rather than growing this PR.
>
> ### Minor
> * **8.** _Test name and placement:_ no JIRA prefix, and it sits between the `SPARK-36680` and `SPARK-50286` cases rather than with the option-forwarding group it is the sequel to. [inline: `sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:84`]
Thanks for the review and detailed response! Addressed 1, 3, 4, 5, 8;
- for 2, talked with @aokolnychyi , since this is an immediate follow up of https://github.com/apache/spark/pull/57582 which got in for 4.3, we will need this to be in 4.3 too
- for 7, this is actually addressed in another https://github.com/apache/spark/pull/57799 , which is part of another follow up for https://github.com/apache/spark/pull/57582.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3667505740
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read, so a catalog can take them into account when producing the {@link Table} or
+ * {@link View}. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; override it to make use of the options.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
+ * @return a {@link Table} for tables, or a {@link View} for views
+ * @throws NoSuchTableException if neither a table nor a view exists at {@code ident}
+ * @since 4.2.0
Review Comment:
```suggestion
* @since 4.3.0
```
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read, so a catalog can take them into account when producing the {@link Table} or
+ * {@link View}. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; override it to make use of the options.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
+ * @return a {@link Table} for tables, or a {@link View} for views
+ * @throws NoSuchTableException if neither a table nor a view exists at {@code ident}
+ * @since 4.2.0
Review Comment:
(or 5.0.0 if targeting master only)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3856210463
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +139,34 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * the user-specified options that may affect table state.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives table-state options. The
Review Comment:
My main concern with requiring the state-aware `loadTable` path is that, for reads carrying state options, it reintroduces the exact table-first / view-second fallback that `RelationCatalog` was added to avoid. It also forces a state-aware `RelationCatalog` implementation back through a table-specific API instead of allowing it to implement the unified lookup directly.
I think a better model would be `loadRelation(Identifier, RelationContext)`. Today, that context would contain only the options projected through `TableCatalog.tableStateOptionKeys()`. Those options would apply only if the result is a `Table` and must be ignored if the result is a `View`. If Spark later introduces view-state options, e.g. to select a version or branch of a view definition, we could add a separate `viewStateOptions` field, projected through a corresponding `ViewCatalog` declaration. The unified call could carry both typed fields and let the catalog apply the one matching the resolved relation kind, preserving the single lookup without conflating table and view state.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5412884785
@yyanyy, replies below.
1. I am not suggesting you would not attempt the view resolution. I was saying you would not be able to exercise the relation fast pass if there are state options. It may very well be a view.
2. I excepted RelationCatalog would overload loadTable to throw an exception when asked for loading with options.
With all that said, let me explore the changes one more time.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3856989922
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +139,34 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * the user-specified options that may affect table state.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives table-state options. The
Review Comment:
Had offline discussion:
1. `loadRelation(Identifier, RelationContext)` puts options inside the context, unlike existing load APIs that always have the consistent `(Identifier, Context, options)` arguments. We may eventually need separate Table and View state options (for not polluting table options from views and enable correct table pinning based on table state), but the view state option mechanism has not been fully designed yet.
2. `loadChangelog(Identifier, ChangelogContext, options)` is an example that shows the compatibility risk of defining an options contract too early, that we need to revisit now that we require `options` to be responsible for table state pinning. We therefore may prefer the conservative approach of requiring users to implement the state-aware loadTable path with loadView fallback for now, and defer the RelationCatalog API until View state is better understood.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3855650812
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +139,34 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * the user-specified options that may affect table state.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives table-state options. The
Review Comment:
We also don't have RelationContext or understanding how it should look like.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5357787558
Question: do we actually have to support state options in RelationCatalog? I think state options today apply only to tables, meaning we can follow what we do today for time travel and write privileges. If we detect state options, don't attempt to use the optimized loading and just try TableCatalog.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3819899362
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -291,7 +291,9 @@ class RelationResolution(
case mc: RelationCatalog
if finalTimeTravelSpec.isEmpty && writePrivileges == null =>
try {
- Some(mc.loadRelation(ident))
+ val stateOptions =
+ CatalogV2Util.extractTableStateOptions(mc, finalOptions)
Review Comment:
**Non-blocking:**
Please reuse this projected map for the subsequent shared-cache lookup. That path currently calls `CatalogV2Util.lookupCachedRelation`, which projects the same option map again, so each uncached relation resolution normalizes the key set and materializes the projection twice.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass all options while loading relations [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3732590089
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +135,26 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * all user-specified options.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives the options passed to the
+ * read. The default implementation ignores {@code options} and delegates to
+ * {@link #loadRelation(Identifier)}; catalogs that want to receive the user options while
+ * reading a relation must override this method.
+ *
+ * @param ident the identifier
+ * @param options all options passed to the read
+ * @return a {@link Table} for tables, or a {@link View} for views
+ * @throws NoSuchTableException if neither a table nor a view exists at {@code ident}
+ * @since 4.3.0
+ */
+ default Relation loadRelation(Identifier ident, CaseInsensitiveStringMap options)
Review Comment:
thanks for the review and the finding! great catch, addressed; was also thinking if I should create other APIs so that we don't check for time travel/write privileges in two different classes but eventually felt that it might be better to not continue adding more loadTable variants.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Use loadTable for RelationCatalog reads with table-state options [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5451689869
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6f568b2f567aea60ade054cca5badcef1d8d0239
- merged into branch-4.x https://github.com/apache/spark/commit/fdc957fecbc4d19f4098a299719ad6b85a4d5df6
- merged into branch-4.3 https://github.com/apache/spark/commit/675fc9836c1504ed51669c04b0833e4b33a81ecb
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58392][SQL] Pass table state options while loading relations [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #57585:
URL: https://github.com/apache/spark/pull/57585#discussion_r3820018491
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:
##########
@@ -789,6 +861,112 @@ class DataSourceV2OptionSuite extends DatasourceV2SQLBase {
}
}
+ test("SPARK-58392: loadRelation receives only state options and scans keep all options") {
Review Comment:
**Finding 14.** The whole point of the projection is that references sharing a table state reuse one `Table` while keeping their own scan options, and this PR moves that door from `loadTable` to `loadRelation`. The three tests that pin it — `same table state shares one Table while preserving each reference's options` (`:1100`), `different table-state option values establish separate table pins` (`:1300`), `SPARK-58389: repeated references with the same options load the table once` (`:1588`) — all go through `loadTable`, and none has a `loadRelation` analog. Every new test here uses exactly one reference, so the call count can never distinguish "projected correctly" from "projected slightly differently from the pin key" (finding 13).
Measured on this head with `StateAwareV2InMemoryRelationCatalog`:
- ``t WITH ('snapshot'='s1', `split-size`=5) a JOIN t WITH ('snapshot'='s1', `split-size`=9) b`` -> **1** `loadRelation` call, `{snapshot=s1}`; two relations carrying `{split-size=5, snapshot=s1}` and `{split-size=9, snapshot=s1}`; one shared `Table` instance.
- `t WITH ('snapshot'='s1') a JOIN t WITH ('snapshot'='s2') b` -> **2** calls, `{snapshot=s1}` and `{snapshot=s2}`.
Both are a handful of lines on the fixture you already have. The second needs per-call assertions rather than `assertOnlySnapshotRelationOptions`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -291,7 +291,9 @@ class RelationResolution(
case mc: RelationCatalog
if finalTimeTravelSpec.isEmpty && writePrivileges == null =>
try {
- Some(mc.loadRelation(ident))
+ val stateOptions =
+ CatalogV2Util.extractTableStateOptions(mc, finalOptions)
Review Comment:
**Finding 13.** `tableKey` is already forced by the time we get here: the branch guard gives `writePrivileges == null`, and `RelationCatalog extends TableCatalog`, so `tableCache.get(tableKey)` at `:274` ran. And `toTableCacheKey` computes `CatalogV2Util.extractTableStateOptions(catalog, finalOptions)` for exactly this `catalog` / `finalOptions` pair (`:630`). So this is a second, independent computation of the value that *is* the pin key — the two have to agree or Spark pins a table under one projection while having loaded it with another. Reusing the key's own projection makes that structural instead of a coincidence, and it is what the description already claims happens ("passes only the catalog-declared `tableStateOptionKeys()` projection (`tableKey.stateOptions`)"):
```scala
try {
Some(mc.loadRelation(ident, tableKey.stateOptions))
} catch {
```
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -134,6 +139,30 @@ public interface RelationCatalog extends TableCatalog, ViewCatalog {
*/
Relation loadRelation(Identifier ident) throws NoSuchTableException;
+ /**
+ * Load the relation for an identifier that may resolve to either a table or a view, forwarding
+ * the user-specified options that may affect table state.
+ * <p>
+ * Behaves like {@link #loadRelation(Identifier)} but also receives table-state options. The
+ * default implementation ignores {@code stateOptions} and delegates to
+ * {@link #loadRelation(Identifier)}; catalogs that want to receive table-state options while
+ * reading a relation must override {@link #tableStateOptionKeys()} and this method.
+ * <p>
+ * Spark calls this for a plain read only -- no time travel and no write privileges, both of
+ * which apply to tables only and route through
+ * {@link TableCatalog#loadTable(Identifier, TableContext, CaseInsensitiveStringMap)} instead.
+ *
+ * @param ident the identifier
+ * @param stateOptions options declared to affect table state
Review Comment:
**Finding 17.** `stateOptions` is documented purely in table terms, and its declaration hook is `TableCatalog#tableStateOptionKeys()` — a table-scoped name — but this method can return a `View`, and Spark applies the same projection to a view lookup. Measured on this head: reading a view with `.option("snapshot", "outer")` calls this with `{snapshot=outer}`. The PR description covers it ("both result kinds receive the same table-state projection"), but the javadoc doesn't, so a connector whose views have their own state selector would read this and reasonably expect either a separate declaration hook or different filtering for views. Same gap as finding 11 — the description is not where implementors look. One sentence closes it:
```java
* @param stateOptions options declared to affect table state by
* {@link #tableStateOptionKeys()}. Spark applies the same table-state
* projection when the identifier turns out to be a {@link View}: the kind
* is only known once this call returns, so there is no separate
* view-state declaration.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala:
##########
@@ -789,6 +861,112 @@ class DataSourceV2OptionSuite extends DatasourceV2SQLBase {
}
}
+ test("SPARK-58392: loadRelation receives only state options and scans keep all options") {
+ registerCatalog("testrelcat", classOf[StateAwareV2InMemoryRelationCatalog])
+ val t1 = "testrelcat.ns1.ns2.table"
+ withTable(t1) {
+ sql(s"CREATE TABLE $t1 (id bigint, data string) USING parquet")
+
+ val relCatalog =
+ catalog("testrelcat").asInstanceOf[StateAwareV2InMemoryRelationCatalog]
+ relCatalog.resetLoadRelationCalls()
+ val df = spark.read
+ .option("SnApShOt", "s1")
+ .option("split-size", "5")
+ .table(t1)
+ val relations = df.queryExecution.analyzed.collect { case r: DataSourceV2Relation => r }
+
+ assertOnlySnapshotRelationOptions(relCatalog, "s1", expectedCalls = 1)
+ assert(relations.size === 1)
+ assert(relations.head.options.size() === 2)
+ assert(relations.head.options.get("snapshot") === "s1")
+ assert(relations.head.options.get("split-size") === "5")
+ }
+ }
+
+ test("SPARK-58392: loadRelation uses the table-state projection for a view") {
Review Comment:
**Finding 6.** R2 closed my original "no view-side coverage" finding with a test that was stronger than what I asked for — [`SPARK-58392: options are forwarded for a view over a V2 table`](https://github.com/apache/spark/blob/58ae83ffcbb45ebf83f72248ec3d73f3261e7e00/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2OptionSuite.scala#L526): a view whose body reads a V2 table in the same catalog, asserting three loads and that the outer read option reached only the view load while the body's own `WITH` option reached only the two table loads. The redesign replaced it with `CREATE VIEW v AS SELECT 1 AS x`, a body with no relation at all, so all that is left is "the projection reaches one view load".
The property still holds — I measured it on this head against `StateAwareV2InMemoryRelationCatalog`:
```
CREATE VIEW v AS SELECT * FROM t WITH ('snapshot' = 'inner')
spark.read.option("snapshot", "outer").option("split-size", "5").table(v).collect()
-> loadRelationCalls = [{snapshot=outer}, {snapshot=inner}, {snapshot=inner}]
```
The outer read's `snapshot` reaches only the view load, and the body's own `snapshot` reaches the body's table loads (analysis + execution refresh). Worth pinning: `createRelation` takes a visibly different branch for `View` (`createDataSourceV1Scan(V1Table.toCatalogTable(...))`, `RelationResolution.scala:448`) and the body's relations are resolved in a nested pass, so nothing else in the suite reaches either. Suggest restoring the R2 test's structure with `snapshot` in place of `split-size` and those three expected bags — note it needs per-call assertions rather than `assertOnlySnapshotRelationOptions`, which requires every recorded bag to carry the same value. Keeping the `SELECT 1 AS x` case alongside it is fine.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/RelationCatalog.java:
##########
@@ -168,7 +197,11 @@ default TableSummary[] listRelationSummaries(String[] namespace)
* <p>
* The default implementation derives from {@link #loadRelation}: a {@link View} is rejected as
* not-a-table; a {@link Table} is returned. Override only if a tables-only path is materially
- * cheaper than the unified one.
+ * cheaper than the unified one -- note that reads do not reach this method: they go to
+ * {@link #loadRelation(Identifier, CaseInsensitiveStringMap)}, directly from the resolver or
+ * via {@link #loadTable(Identifier, TableContext, CaseInsensitiveStringMap)}. What remains
+ * here is write-privilege loads, DDL and miscellaneous lookups and reloads, including
Review Comment:
**Finding 15.** My R3 finding-11 comment listed "the `V2TableReference` reload (`RelationResolution.scala:510`)" and you wrote that in, so this one is mine to correct. Only the *non-cacheable* `WriteTargetContext` branch reaches `loadTable(Identifier)` (`RelationResolution.scala:570`). The two cacheable contexts — `TemporaryViewContext` and `TransactionContext` (`V2TableReference.scala:97`, `:103`) — go through `getOrLoadRelation` -> `CatalogV2Util.getTable` -> `:230` -> `loadRelation(ident, stateOptions)`, exactly like every other read. And that one branch isn't a "write-privilege load" either: `RelationResolution.scala:567-569` says it carries no privileges. So:
```java
* here is write-privilege loads, DDL and miscellaneous lookups, and the non-cacheable
* {@code V2TableReference} write-target reload.
```
(Not asking you to change that reload's behavior — that stays with the follow-up the comment there points at.)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org