srielau commented on PR #55523:
URL: https://github.com/apache/spark/pull/55523#issuecomment-4329814664
Addressed @vladimirg-db's review threads in commit `4edb448dbf8`.
> I still don't understand why we need to store conf in the Resolver. This
is not correct. Views/udfs override this and it becomes stale.
> conf is a thread local. why pass it to FunctionResolver at all
Agreed -- the captured-conf pattern is wrong. View/UDF resolution explicitly
nests a new `SQLConf` onto the thread-local via `SQLConf.withExistingConf(...)`
(Analyzer.scala:2517 for SQL functions, :2816 for views), so a
constructor-captured `conf` field on `Resolver` / `FunctionResolution` /
`RelationResolution` reads the *outer* conf during nested resolution, not the
active one. Reads like `pathEnabled`, `prioritizeSystemCatalog`,
`sessionLocalTimeZone`, `caseSensitiveAnalysis` (`conf.resolver`), and the
time-travel keys can legitimately differ between session conf and view-body
conf, so the staleness is observable.
Changes in `4edb448dbf8`:
- `FunctionResolution`: drop `conf` constructor param; mix in
`SQLConfHelper` so `conf.pathEnabled` / `conf.prioritizeSystemCatalog` read
`SQLConf.get` (live thread-local).
- `RelationResolution`: drop `sessionConf` constructor param and the
`override def conf`. `SQLConfHelper.conf = SQLConf.get` covers all reads.
- `Resolver`: drop `conf` constructor param; replace
`conf.maxToStringFields` sites with `SQLConf.get.maxToStringFields`.
- `Analyzer`: stop threading `sessionConf` / `resolutionConf` into the
resolution-class constructors. Keep `Analyzer.sessionConf` and the
`SQLConf.withExistingConf(c) { ... }` wrap in `executeAndCheck` /
`executeSameContext` -- that's the legitimate mechanism that establishes the
active conf for the whole analyzer run; nested view/UDF blocks correctly push
their own conf on top of that.
- `TimezoneAwareExpressionResolverSuite`: drop the now-removed third arg to
`new FunctionResolution(...)`.
Diff: 5 files, 22 lines removed, 10 added. Local compile/test was not
possible in my dev environment due to a Maven proxy gap on the new `guava
33.6.0-jre`; relying on CI here.
--
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]
