[vuanhphung]

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.

[peco-review-bot]

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.

[peco-review-bot]

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.

[peco-review-bot]

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).

[vuanhphung]

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.

[peco-review-bot]

✅ No issues identified by the review bot.