peter-toth commented on PR #57582:
URL: https://github.com/apache/spark/pull/57582#issuecomment-5103091206
Heads-up on an overlap: @anuragmantri has #57508 (SPARK-58330) open, and it
changes the same lines this PR touches.
#57508 fixes the case where the same table appears more than once in a
statement with different `WITH (...)` options. The per-query relation cache is
keyed on `(catalog, identifier, timeTravelSpec)` only, so the second reference
gets a cache hit and silently inherits the first reference's options. Its fix
re-applies each reference's own options to the cached relation on a hit.
Mechanically the two will conflict: #57508 restructures the `.orElse { val
finalOptions = ... }` block that this PR reads `finalOptions` from. (Minor and
separate: this PR and #57585 conflict with each other on the import line in
`DataSourceV2OptionSuite`, though their `RelationResolution` edits merge fine.)
**The design question.** A cache hit never calls the catalog. So once
`loadTable` can produce a different `Table` depending on the options, a second
reference with different options keeps the `Table` built for the first one —
its schema, partitioning, capabilities, and whatever the catalog selected from
those options. #57508 can correct the `options` field on the cached relation,
but it cannot re-derive the `Table`. Which reference wins is analyzer traversal
order — for `INSERT` the target resolves after its query.
Spark already has a worked example of an option that affects the load, and
it's instructive: time travel can be given purely as an option, `versionAsOf` /
`timestampAsOf` (key names configurable via `spark.sql.timeTravelVersionKey` /
`timeTravelTimestampKey`). `TimeTravelSpec.fromOptions` parses it out of the
read options, and then Spark does three things with it — puts it in the
relation cache key, dispatches at load time to `loadTable(ident, version)`, and
forbids it on a write target. It deliberately does *not* leave it to
`newScanBuilder(options)`, and it can't: `DataSourceV2Relation.output` comes
from the `Table` returned at load time, so a snapshot selector that only
reached the scan builder would have the query analyzed against the current
schema and scanned at the old one.
So the question for this API is which kind of option it is meant to carry:
- **Options that change how the table is read** (split size and the like):
`DataSourceV2Relation.options` is the right home, that is already
per-reference, and #57508 is the complete fix — nothing needed here.
- **Options that change what the table is** (a snapshot, a branch, a
schema): they need the `versionAsOf` treatment — cache key, load-time dispatch,
rejected on write targets. Today a connector's own selector of this kind is
invisible to `fromOptions`, so it gets none of the three, and this PR is what
makes it actionable at load time.
The javadoc currently says both — "take them into account when producing the
`Table`" and "customize the scan" — which is what leaves it ambiguous.
The two treatments are not interchangeable, either. Putting options in the
cache key means one load per distinct option bag, which gives up
resolve-once-per-query: `MERGE INTO t WITH (a) USING t WITH (b)` would call
`loadTable` twice, and each call resolves the table's state independently, so
whether the target and the source observe the same state becomes the
connector's problem rather than Spark's guarantee. #57508's approach keeps one
`Table` for the statement and varies only the options. Worth picking
deliberately.
**Separate point on the same refactor.** `CatalogV2Util.getTable` currently
has:
```scala
assert(writePrivilegesString.isEmpty, "Should not write to a table with time
travel")
```
This PR removes it, and the new default overload replaces it with silent
precedence:
```java
if (context.timeTravel().isPresent()) { ... }
else if (!context.writePrivileges().isEmpty()) { ... }
```
If both are ever set, time travel wins and the write privileges are dropped,
so the catalog is never asked to authorize the write — the same shape as
SPARK-58370. I couldn't find an earlier analyzer guard, so as far as I can tell
that assert was the enforcement. It may be unreachable from SQL via the grammar
today, in which case keeping the assert seems better than silently choosing
one. A connector overriding the new method also receives a `TableContext` that
can hold both, with no word on what the combination means.
Last thing: neither this PR nor #57585 has a test with the same table
referenced twice, and the `lastLoadTableOptions` fixture records only the last
call, so it could not express one.
The same argument applies to #57585 (`RelationCatalog.loadRelation`) — no
need to repeat it there.
cc @anuragmantri
--
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]
