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