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: [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]
