srielau commented on PR #55717: URL: https://github.com/apache/spark/pull/55717#issuecomment-4398702601
## Review (SPARK-56750 / default PATH conf) ### Overall Strong direction: **`spark.sql.defaultPath`** mirrors default-catalog style admin defaults; **shared PATH grammar** for conf vs `SET PATH` avoids drift; **`DEFAULT_PATH` cycle break**, **duplicate validation** shared with `SET PATH`, and **case-insensitive `system.*` path entries** are all solid. Wiring **unqualified function shortcuts** (COUNT rewrite, temp-vs-builtin security) to the **live effective PATH** via `CatalogManager` fixes a real semantic gap vs `spark.sql.functionResolution.sessionOrder` alone. ### Deadlock / locking Removing `synchronized` from the outer `lookupBuiltinOrTempTableFunction` / `lookupFunctionInfo` while keeping narrower sync for temp/view-context paths is a reasonable fix for **SessionCatalog ⇄ CatalogManager** lock order. **Follow-up suggestion:** a one-line Scaladoc note that a single lookup is **best-effort consistent** if PATH or temp registry changes mid-flight—sets expectations for reviewers and future readers. ### Other follow-ups (non-blocking) - **`confDefaultPathElementCache`:** confirm any path that mutates `spark.sql.defaultPath` without a full catalog reset still invalidates or replaces the cache as needed (session SET likely OK). - **`SQLConf.DEFAULT_PATH` doc:** double-check the empty-default story matches code (`defaultPathOrder` / `sessionFunctionResolutionOrder` vs `sessionPathEntries` when PATH is on and both stored path and conf are empty). - **MiMa / API:** if `PathElement` moved packages, note any connector/binary compatibility expectations (if it was previously only `private[sql]`, probably fine). ### DDL fix Scoping **`DROP TEMPORARY FUNCTION`** builtin collision to **unqualified** + **`!ifExists`** and delegating to `dropTempFunction` for `IF EXISTS` matches the described bug and looks correct. ### Tests `SetPathSuite` coverage for lazy fallback, overrides, `SET PATH = DEFAULT_PATH`, cycle break, invalid conf, PATH off, PATH-driven security, user-only PATH (no implicit builtins), and duplicates is appropriate. No blockers from this pass; **approve with minor doc/concurrency nits** above if useful. -- 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]
