Skip to content

[SPARK-58389][SQL] Pass all options while loading tables - #57582

Open
yyanyy wants to merge 1 commit into
apache:masterfrom
yyanyy:spark-dsv2-loadtable-options-20260727
Open

[SPARK-58389][SQL] Pass all options while loading tables#57582
yyanyy wants to merge 1 commit into
apache:masterfrom
yyanyy:spark-dsv2-loadtable-options-20260727

Conversation

@yyanyy

@yyanyy yyanyy commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This is a follow-up to #56044 (which passed all options while loading changelogs). It does the same for table reads by adding TableCatalog.loadTable(Identifier, TableContext, CaseInsensitiveStringMap), where TableContext carries the parsed, Spark-recognized parameters (time travel, write privileges) and the CaseInsensitiveStringMap carries all raw user options.

  • New public connector types TableContext and TimeTravel (a clean sealed interface with Version/Timestamp records), mirroring how [SPARK-56961][SQL] Pass all options while loading changelog #56044 introduced ChangelogContext/ChangelogRange rather than leaking the catalyst-internal TimeTravelSpec.
  • The new loadTable overload has a default implementation that delegates to the existing loadTable overloads based on TableContext, so existing connectors keep working unchanged.
  • CatalogV2Util.getTable/loadTable now build a TableContext from the catalyst TimeTravelSpec + write-privileges string and forward the user options, making the Java default the single dispatch site.
  • Options are threaded through the read paths in RelationResolution and DataSourceV2Utils.

Potential follow-up (intentionally out of scope here): the RelationCatalog single-RPC loadRelation(Identifier) path is not updated to forward options. It applies only to catalogs that expose both tables and views, and fires only when there is no time travel or write privileges; closing that gap cleanly would add a loadRelation(Identifier, CaseInsensitiveStringMap) overload and is better done as a separate change.

Why are the changes needed?

To make the API usable in connectors like Iceberg and Delta, which need the user options while reading a table. The functionality hasn't been released yet.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New tests in CatalogV2UtilSuite (the default-dispatch mapping for each TableContext shape, and the time-travel/write-privileges mutual-exclusion invariant) and DataSourceV2OptionSuite (end-to-end option forwarding via the DataFrame API, SQL, streaming, time travel, and the write path). Existing SupportsCatalogOptionsSuite, ChangelogResolutionSuite, ChangelogEndToEndSuite, and the DataSourceV2 SQL/DataFrame suites pass.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude code Opus 4.8

### What changes were proposed in this pull request?

This is a follow-up to apache#56044 (which passed all options while loading
changelogs). It does the same for table reads by adding
`TableCatalog.loadTable(Identifier, TableContext, CaseInsensitiveStringMap)`,
where `TableContext` carries the parsed, Spark-recognized parameters
(time travel, write privileges) and the `CaseInsensitiveStringMap` carries
all raw user options.

- New public connector types `TableContext` and `TimeTravel` (a clean
  sealed interface with `Version`/`Timestamp` records), mirroring how
  apache#56044 introduced `ChangelogContext`/`ChangelogRange` rather than leaking
  the catalyst-internal `TimeTravelSpec`.
- The new `loadTable` overload has a default implementation that delegates
  to the existing `loadTable` overloads based on `TableContext`, so existing
  connectors keep working unchanged.
- `CatalogV2Util.getTable`/`loadTable` now build a `TableContext` from the
  catalyst `TimeTravelSpec` + write-privileges string and forward the user
  options, making the Java default the single dispatch site.
- Options are threaded through the read paths in `RelationResolution` and
  `DataSourceV2Utils`.

This PR does not touch the `RelationCatalog` single-RPC `loadRelation(Identifier)`
read path, which for a table-and-view catalog is the primary path for a plain
read (no time travel / write privileges) and so does not forward options. That
is an independent improvement -- it needs nothing from this change (a new
`loadRelation(Identifier, CaseInsensitiveStringMap)` overload plus wiring) --
and will be a separate PR.

### Why are the changes needed?

To make the API usable in connectors like Iceberg and Delta, which need the
user options while reading a table. The functionality hasn't been released
yet.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

New tests in `CatalogV2UtilSuite` (the default-dispatch mapping for each
`TableContext` shape, and the time-travel/write-privileges mutual-exclusion
invariant) and `DataSourceV2OptionSuite` (end-to-end option forwarding via the
DataFrame API, SQL, streaming, time travel, and the write path). Existing
`SupportsCatalogOptionsSuite`, `ChangelogResolutionSuite`,
`ChangelogEndToEndSuite`, and the DataSourceV2 SQL/DataFrame suites pass.
@peter-toth

peter-toth commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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 [SPARK-58330][SQL] Prevent silent dropping of dynamic options on same table references. #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.

Side note on a problem this creates regardless of which way it goes: the single-pass resolver keys on RelationId(multipartIdentifier, options, isStreaming, timeTravelSpec), so it already resolves once per option bag while the fixed-point relation cache does not. Once a connector overrides the new overload, the same query would resolve differently under the two.

Separate point on the same refactor. CatalogV2Util.getTable currently has:

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants