srielau commented on PR #55717: URL: https://github.com/apache/spark/pull/55717#issuecomment-4391182264
## Review follow-up (SPARK-56750 / #55717) Thanks for the detailed review — here is how the latest commits address it. ### Correctness / threading - **Deadlock (SC ⇄ CM):** Removed `synchronized` from `SessionCatalog.lookupBuiltinOrTempTableFunction` so the temp metadata path does not hold `SessionCatalog` while invoking `sessionFunctionKindsProvider` → `CatalogManager` (re-entrant `synchronized` on CM is still possible; the AB-BA lock order with `setCurrentNamespace` is avoided). - **`sessionFunctionKindsProvider` default:** Restored the **`SESSION_FUNCTION_RESOLUTION_ORDER`**-based default when no `CatalogManager` wiring is present (direct `SessionCatalog` in tests). - **`@volatile`** on the provider `var` for safe publication after `CatalogManager` ctor wiring. ### PATH → builtin/session “shortcut” semantics - **`\"last\"` mode / DESCRIBE / quick-check:** System kinds are taken from the **prefix before the first user-catalog entry** when that prefix contains any `system.*` entries (legacy builtin-only shortcut preserved). - If that prefix has **no** system entries but the **full** path does (e.g. user schema before `system.builtin`), kinds come from the **full** path (no magic default list). - **Removed** the implicit **`Seq(Builtin, Temp)`** fallback when no system segments match; **added** a test that a user-only PATH does **not** resolve unqualified builtins (`UNRESOLVED_ROUTINE` for `abs`). - **`isSystemBuiltinPathEntry` / `isSystemSessionPathEntry`:** Now **case-insensitive** so stored paths like `System.Builtin` still classify as system entries (fixes empty kinds + `current_path()` for case-preserved literals). ### Performance / naming / validation - **`spark.sql.defaultPath`:** Renamed `workspace*` → **`confDefaultPath*`** / **`isConfDefaultExpansion`**; **`parseSqlPathElements` → `parsePathElements`** on `AbstractSqlParser`. - **Parse cache:** Cached **parsed `PathElement` list** by conf string (ANTLR only); expand + duplicate validation still run on each materialization so `CURRENT_SCHEMA` / live catalog stay correct. - **Duplicates:** Shared **`PathElement.validateNoDuplicateResolvedPath`** for `SET PATH` and conf default materialization (same rules as command run). - **Single-part schema in PATH:** Validated in **`AstBuilder.visitPathElement`** (fail at parse/analysis with path position), not on every `expand`. - **`DEFAULT_PATH`:** **`.checkValue`** parses via `parsePathElements`; doc string fixed (no broken `[[...]]` in user-facing text; references `spark.sql.functionResolution.sessionOrder` by key). ### `DROP TEMPORARY FUNCTION IF EXISTS session.count` (cleanup in tests) - This was **not** the analyzer binding `session.count` to the builtin. **`DropFunctionCommand`** stripped to `count` and then threw **`cannotDropBuiltinFuncError`** when no temp existed but a builtin collided — **before** calling `dropTempFunction`, which made `IF EXISTS` cleanup fail incorrectly. - **Fix:** Removed that guard; **`dropTempFunction` only touches the temp registry** and already honors `ignoreIfNotExists`. Tests use SQL **`DROP TEMPORARY FUNCTION IF EXISTS session.count`** again. ### Non-action / follow-up - **“Coordination vs strategy”** note on where `leadingSystemFunctionKinds` lives: acknowledged; left as-is for this PR to avoid a larger refactor (happy to move toward `FunctionResolution` in a follow-up if reviewers want that split explicitly). If anything above should be tweaked before merge, call it out inline and I will adjust. -- 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]
