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