Skip to content

feat: add opt-in metadata catalog name escaping - #985

Merged
vuanhphung merged 3 commits into
mainfrom
vu-phung/escape-metadata-catalog-names
Oct 15, 2026
Merged

vuanhphung merged 3 commits into
mainfrom
vu-phung/escape-metadata-catalog-names

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • Feature

Description

Catalog names containing _ can trigger unintended metadata pattern matching and extra catalog lookups. Add the opt-in escape_catalog_name connection parameter, defaulting to False, to escape unescaped underscores while preserving existing escapes and % wildcards.

This applies to schemas, tables, and columns on Thrift and tables on kernel. Kernel schemas and columns already use exact catalog identifiers.

How is this tested?

  • Unit tests

628 tests passed, 3 skipped, including 198 metadata regression cases across both backends. Explicit cases document JDBC compatibility for even/odd backslash runs. Kernel calls are mocked. Black formatting and git diff --check passed.

Related Tickets & Documents

N/A

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a clean, well-tested opt-in feature. End-to-end wiring (connect → session → both backends) is correct, the Thrift/Kernel asymmetry (escaping on schemas/tables/columns vs. tables only) matches the documented behavior and the _catalog_or_none normalization, and the parametrized unit tests cover both backends thoroughly. One minor low-severity edge case noted in the escaping regex.

Comment thread src/databricks/sql/utils.py
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — clean, opt-in, default-off feature that's well wired (Thrift via **kwargs, kernel explicitly in session.py) and thoroughly parametrized in tests. One low note: the escape regex only checks a single preceding backslash, so the "Match JDBC … regardless of backslash parity" comment doesn't actually match JDBC/LIKE semantics for even-length backslash runs (harmless in practice since catalog names lack backslashes). Also worth being aware: the parameter is a silent no-op on the SEA backend and on kernel schemas()/columns() by design — this matches the documented scope, so no change needed, just flagging for intent.

Comment thread src/databricks/sql/utils.py
@vuanhphung
vuanhphung enabled auto-merge October 15, 2026 22:28
@vuanhphung
vuanhphung added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit b1978d4 Oct 15, 2026
52 checks passed
@vuanhphung
vuanhphung deleted the vu-phung/escape-metadata-catalog-names branch October 15, 2026 23:25

This branch was successfully deployed

1 active deployment
azure-prod — 78790895 Deployed Oct 15, 2026 by peco-review-bot[bot] via followup #989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants