GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Escape `_` in metadata catalog_name by default (behavior change) by vuanhphung · Pull Request #1029 · databricks/databricks-sql-python

Conversation

vuanhphung commented Nov 12, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

What type of PR is this?

Bug Fix
Other (behavior change, performance)

Description

Behavior change: escape_catalog_name (added opt-in in #985) now defaults to True. _ in catalog_name is no longer a single-character wildcard in Thrift schemas()/tables()/columns() or in kernel tables(). % still works as a wildcard, and escape_catalog_name=False restores the old behavior. The JDBC/ODBC metadata spec treats the catalog as an exact name, not a pattern, and the Databricks JDBC driver has escaped _ by default since v3.3.1 (TreatMetadataCatalogNameAsPattern=0). Kernel schemas()/columns() and SEA already match the exact name, so this makes all backends agree by default.

Correctness: the server treats Thrift catalogName as a LIKE pattern, so today an unescaped _ can return rows from other catalogs. On our test workspace, cursor.columns(catalog_name="comparator_tests") returns 631 rows from comparator_tests plus 488 rows from comparator-tests. With this change it returns only the 631.

hive_metastore returns identical rows with and without escaping on both backends. spark_catalog improves: on Thrift, schemas(catalog_name="spark_catalog") used to return nothing, because the unescaped pattern matches no listed catalog. Escaped, it resolves to hive_metastore, as kernel already does.

Latency: the same pattern makes the server scan every catalog. That is the ~200 ms Thrift-vs-kernel gap in the nightly metadata benchmark, whose fixture catalog is jdbcbenchmarkingcatalog_<id>. The client side is already minimal: each call is one RPC with full direct results and 1–2 ms of client CPU. Query History shows 215–263 ms of server time unescaped vs 38–92 ms with \_. tables() showed no gap only because kernel tables() sent the same pattern.

Median (p95) ms on a live serverless Medium warehouse, catalog jacky_test (3 schemas), nightly-suite arguments. Fresh processes ran main and this branch interleaved, 6 runs × 5 calls per side. Row counts are identical.

call	Thrift unescaped (main)	Thrift escaped (this PR)	kernel (main)	kernel (this PR)
`schemas(cat, "%")`	273 (432)	106 (146)	118 (141)	116 (133)
`schemas(cat, "def%")`	267 (300)	104 (154)	114 (162)	113 (162)
`tables(cat, "default", "%")`	335 (384)	175 (262)	448 (513)	172 (235)
`columns(cat, "default", "customer", "%")`	271 (337)	104 (158)	113 (135)	115 (186)
`columns(cat, "%", "%", "%")`	448 (601)	253 (292)	279 (347)	283 (350)
control: `schemas("peco", "def%")`, no `_`	288 (357)	294 (407)	297 (351)	303 (385)

How is this tested?

Unit tests
E2E Tests
Manually

The unit tests now state the expected default explicitly, and the backend-level forwarding tests assert the escaped name. The full unit suite and -m realkernel pass. New e2e tests run on Thrift and kernel:

syste_ matches nothing by default and matches system with the opt-out.
On Thrift, an escaped underscore catalog returns exactly the unescaped rows for that catalog, which shows the server honours \_.
hive_metastore schemas()/tables() (Thrift) and tables() (kernel) return the same rows with and without escaping, and spark_catalog loses no rows.

The new e2e tests fail on main and pass here. The existing Thrift and kernel metadata e2e tests also pass on the live warehouse.

Related Tickets & Documents

#985

This pull request and its description were written by Isaac.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix(metadata)!: escape _ in catalog_name by default

1d52f23

Behavior change: escape_catalog_name now defaults to True, so `_` in
catalog_name is no longer a single-character wildcard for Thrift
schemas()/tables()/columns() or kernel tables(). `%` still works, and
escape_catalog_name=False restores the old behavior. This follows the
JDBC/ODBC metadata spec (catalog is an exact name) and the Databricks
JDBC driver's default since v3.3.1.

The server treats Thrift catalogName as a LIKE pattern, so an unescaped
`_` returned rows from other catalogs (comparator-tests for
comparator_tests) and made the server scan every catalog (~170 ms per
call on underscore catalogs).

Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:02

— with

GitHub Actions Error

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:02

— with

GitHub Actions Inactive

peco-review-bot Bot approved these changes

Nov 12, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 2 Low

Looks good, with two low-severity points. The new default is applied the same way at all three sites (Thrift, kernel client, session forwarding), and the unit and e2e tests cover the default, opt-out and opt-in cases. The two findings are an opt-out check that a string like "false" doesn't actually disable, and a tables() docstring that is less precise than the others.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sql/backend/thrift_backend.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/databricks/sql/client.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:04

Inactive

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:04

Inactive


          docs: scope tables() catalog wildcard opt-out to Thrift and kernel

86a6401

Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:06

— with

GitHub Actions Inactive

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:06

— with

GitHub Actions Inactive

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:06

— with

GitHub Actions Error

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:06

— with

GitHub Actions Error

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:06

— with

GitHub Actions Error

peco-review-bot Bot had a problem deploying to azure-prod

November 12, 2026 08:07

Error

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:07

Inactive

peco-review-bot Bot reviewed

Nov 12, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Medium

The default flip itself looks right: all three default sites (Thrift, kernel client, session forwarding) are updated together, and the docs and tests agree with the new default. I have one medium concern. The new e2e test skips hive_metastore, which is the most common catalog name with _, so nothing checks that escaping it by default doesn't break metadata calls on that catalog.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/e2e/test_driver.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:08

Inactive


          test(e2e): cover hive_metastore and spark_catalog with escaped catalo…

ed49b2e

…g names

Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:14

— with

GitHub Actions Inactive

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:14

— with

GitHub Actions Inactive

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:15

— with

GitHub Actions Error

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:15

— with

GitHub Actions Error

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:19

Inactive

peco-review-bot Bot approved these changes

Nov 12, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Low · 1 Nit

The default flip looks correct, and docs, CHANGELOG and tests were all updated consistently. I found two low-severity issues: an explicit escape_catalog_name=None (or a string value) now resolves differently from the new default, and the spark_catalog e2e assertion can't fail on the improvement the PR claims. A nit outside this diff: the regex in escape_metadata_catalog_name uses a one-character lookbehind (it ignores whether the preceding backslash run is even or odd), and that approximation now runs on every catalog name by default.

Other findings

⚪ Nit — The PR description says escaping now makes Thrift schemas(catalog_name="spark_catalog") resolve to hive_metastore, where before it returned nothing. This assertion (rows(raw, ...) <= rows(escaped, ...)) passes trivially when both sides are empty. It also passes if escaping still returns nothing for spark_catalog, so it never checks the improvement the PR claims. If that behavior is meant to be guaranteed, also assert that the escaped result is non-empty (skip when hive_metastore.default isn't visible, as the earlier block already does).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sql/backend/thrift_backend.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

peco-review-bot Bot had a problem deploying to azure-prod

November 12, 2026 08:19

Error


          fix(metadata): treat escape_catalog_name=None as the default

268b6bc

An explicit None (e.g. from a wrapper forwarding unset options) used to
match the old False default; keep it meaning "not set" now that the
default is True. Also assert that escaped spark_catalog resolves.

Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>

vuanhphung commented Nov 12, 2026

Copy link

Copy Markdown

Collaborator Author

Review-body nits from the round on ed49b2e:

spark_catalog assertion passes trivially: fixed in 268b6bc. The test now also asserts that escaped schemas(catalog_name="spark_catalog", schema_name="default") is non-empty. It already skips when hive_metastore.default is not visible.
One-character lookbehind in escape_metadata_catalog_name: not changing it here. This PR does not touch the function. Its output is identical to the JDBC driver's WildcardUtil.escapeCatalogName, which keeps any \_ pair and escapes every other _, so a\\_b stays a\\_b in both. The existing jdbc-even-backslash-run and jdbc-odd-backslash-run unit cases pin that. It only affects catalog names with a backslash right before _. Diverging from JDBC there would be its own behavior change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:23

— with

GitHub Actions Error

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:23

— with

GitHub Actions Inactive

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:23

— with

GitHub Actions Error

vuanhphung had a problem deploying to azure-prod

November 12, 2026 08:23

— with

GitHub Actions Error

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:24

Inactive

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:24

— with

GitHub Actions Inactive

peco-review-bot Bot approved these changes

Nov 12, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

✅ No issues identified by the review bot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vuanhphung temporarily deployed to azure-prod

November 12, 2026 08:28

— with

GitHub Actions Inactive

peco-review-bot Bot temporarily deployed to azure-prod

November 12, 2026 08:31

Inactive

vuanhphung temporarily deployed to azure-prod

November 12, 2026 09:07

— with

GitHub Actions Inactive

This branch was previously deployed

1 inactive deployment

azure-prod — 268b6bcd Deployed Nov 12, 2026 by vuanhphung via followup #1854

Reviewers

peco-review-bot[bot]

peco-review-bot[bot] approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

vuanhphung commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

Uh oh!

peco-review-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

peco-review-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peco-review-bot Bot left a comment

Choose a reason for hiding this comment

Other findings

Uh oh!

Uh oh!

vuanhphung commented Nov 12, 2026

Uh oh!

peco-review-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vuanhphung commented Nov 12, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.