fix(kernel): preserve empty metadata filters - #933
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Clean, well-scoped change that stops collapsing empty/blank metadata filters to None and preserves them as real (match-nothing) patterns, with docstrings/CHANGELOG/tests updated to match. One medium concern: the correctness of the new "empty string matches nothing" behavior rests on kernel semantics that the removed comment described as the opposite (kernel rejecting "" with InvalidArgument), and the only tests exercising it are live-warehouse e2e tests — worth confirming against the real kernel and checking whether the ^0.2.0 pin needs bumping.
79b87d7 to
74d883f
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Focused, well-tested fix that flips empty metadata filters from match-all to match-nothing. One medium concern: get_tables passes an empty catalog straight to the kernel while get_schemas/get_columns adapt it via _exact_catalog_and_pattern, and no test verifies tables(catalog_name="") actually matches nothing — worth confirming the kernel's list_tables doesn't treat blank as "all catalogs."
GitHub Actions
Failure
There was a problem hiding this comment.
Verdict: 1 High · 1 Low
One real concern: get_tables is the odd one out — it passes catalog=catalog_name unbridged while get_schemas/get_columns route empty catalogs through the new _exact_catalog_and_pattern helper, so tables(catalog_name="") likely matches all catalogs (or raises) instead of "matches nothing" as the PR's own docstring promises (F1, high). Test coverage for the empty-catalog tables case is also missing (F2, low). The schema/table/column pattern preservation and the empty-catalog bridge for schemas/columns look correct.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-tested behavior change that stops collapsing empty/blank metadata filters to None and forwards them to the kernel unchanged. Unit + e2e coverage is thorough and no references to the removed _none_if_blank/_catalog_or_none helpers remain. One low-severity docstring-consistency nit around how catalog_name is described across the three metadata methods.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Focused, well-tested change that stops collapsing empty/whitespace metadata filters to None on the kernel path. Two concerns: (1) the "empty pattern matches nothing" contract now depends on kernel behavior the connector no longer controls — the removed helper documented that the kernel rejects "" with ProgrammingError, and mocked unit tests can't detect a mismatch, so please confirm/pin the kernel version; (2) the shared abstract base-class docstring now documents kernel-only %/* catalog semantics that don't hold for the Thrift backend.
| stream = self._kernel_session.metadata().list_schemas( | ||
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| schema_pattern=schema_name, |
There was a problem hiding this comment.
🟡 Medium — The change now forwards empty-string pattern filters (schema_pattern, table_pattern, column_pattern) to the kernel verbatim instead of collapsing them to None. The PR description, the base/Cursor docstrings, and the e2e tests all assert that an empty pattern matches nothing (fetchall() == []).
But the previous helper's own docstring documented the opposite kernel behavior: Identifier/LikePattern reject "" with InvalidArgument, which the connector maps to ProgrammingError. If the pinned kernel (databricks-sql-kernel = "^0.2.0", pyproject.toml:62) still rejects "", then empty-string filters now raise ProgrammingError at runtime rather than matching nothing — a regression relative to the documented/asserted contract.
The unit tests (test_get_schemas_preserves_empty_pattern, etc.) mock the kernel session, so they only verify the connector passes "" through — they cannot detect that the real kernel rejects it. Only the real-wheel e2e tests would, and those run in a separate CI step. Please confirm this depends on a coordinated kernel change that accepts "" as match-nothing, and bump the minimum databricks-sql-kernel version (^0.2.0) accordingly so a user on an older kernel doesn't silently get ProgrammingError where the docs promise an empty result set. Applies equally to the call sites at lines 945 and 975-977.
(Anchored to the nearest changed line — see the description for the exact location.)
| catalog_name: Optional catalog name pattern to filter by | ||
| schema_name: Optional schema name pattern to filter by | ||
| catalog_name: Optional exact catalog name. ``None`` leaves the | ||
| filter unset; ``%`` and ``*`` select all catalogs; an empty |
There was a problem hiding this comment.
🔵 Low — This docstring lives on the abstract DatabricksClient base class, which is the shared contract for both the Thrift and kernel backends. It now states catalog_name is an "Optional exact catalog name" where "% and * select all catalogs." That semantics is kernel-only: the Thrift backend passes catalogName=catalog_name straight through (thrift_backend.py:1160/1206/1254) with no %/* normalization, so on Thrift % is a literal catalog name — as the removed _catalog_or_none comment itself noted ("This intentionally diverges from raw-Thrift literalness (Thrift treats % as a literal catalog name)").
A reader of the base contract (and Thrift users) will be misled into thinking catalog_name='%' matches all catalogs on every backend. Consider scoping the wildcard note to the kernel backend, or clarifying that it is a kernel-specific normalization. The same wording appears at databricks_client.py:332 (get_columns) and in the public Cursor docstrings at client.py:1583-1584 and 1641-1642, which are likewise backend-agnostic and user-facing.
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall — a focused, well-tested semantics change (empty metadata filters preserved rather than collapsed to None), with unit + e2e coverage and CHANGELOG/docstring updates. Two low-severity notes: a docstring inconsistency for get_tables.catalog_name vs. the other two methods, and a potential Thrift/kernel parity divergence on empty-string filters (the removed helper claimed the old collapse matched Thrift) that lacks a cross-backend test.
| cursor: The cursor object that will handle the results | ||
| catalog_name: Optional catalog name pattern to filter by | ||
| if catalog_name is None, we fetch across all catalogs | ||
| if catalog_name is None, we fetch across all catalogs; an empty |
There was a problem hiding this comment.
🔵 Low — The get_tables docstring for catalog_name is inconsistent with the parallel updates to get_schemas and get_columns. Here it still reads "Optional catalog name pattern to filter by ... an empty string matches nothing", but the other two methods were updated to describe catalog as "Optional exact catalog name ... % and * select all catalogs; an empty string is preserved."
Catalog is not treated as a LIKE pattern (it flows through _catalog_or_none, which only special-cases %/*), so an empty catalog string is preserved and forwarded as an exact identifier — the same as in get_schemas/get_columns, not "matches nothing as a pattern." Align the wording so callers aren't told catalog behaves like a pattern in one method and an exact name in the others.
14713e6 to
95ec810
Compare
GitHub Actions
Failure
There was a problem hiding this comment.
Verdict: 1 Low
Clean, well-scoped fix: the kernel adapter now passes schema/table/column filters through verbatim (empty patterns match nothing) instead of collapsing blanks to None, with _catalog_or_none still normalizing %/*; the removed _none_if_blank helper has no remaining references, and unit + e2e tests were updated to lock the new semantics. One Low doc-consistency nit: the get_tables catalog_name docstring wasn't aligned with its siblings and may misdescribe empty-catalog behavior.
| cursor: The cursor object that will handle the results | ||
| catalog_name: Optional catalog name pattern to filter by | ||
| if catalog_name is None, we fetch across all catalogs | ||
| if catalog_name is None, we fetch across all catalogs; an empty |
There was a problem hiding this comment.
🔵 Low — The get_tables catalog_name docstring is inconsistent with its sibling methods and with the actual code path. get_schemas (257-259) and get_columns (337-339) were updated to describe catalog_name as an exact name where "% and * select all catalogs; an empty string is preserved." But get_tables here still calls it a "pattern", omits the %/* → all-catalogs behavior, and states "an empty string matches nothing."
All three methods apply the same _catalog_or_none(catalog_name) normalization in the kernel backend (client.py:924/951/981), so the catalog handling is identical across them — the divergent wording is misleading. Moreover, per the comment removed in this PR, the kernel treats a blank/%/* catalog as all catalogs (is_null_or_wildcard) for SHOW TABLES, so "an empty string matches nothing" for the catalog on get_tables appears to contradict actual behavior. Recommend aligning this docstring with the get_schemas/get_columns wording.
(Anchored to the nearest changed line — see the description for the exact location.)
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
95ec810 to
0f8a432
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Straightforward, well-tested behavior change: empty/whitespace metadata filters are no longer collapsed to None, patterns pass through verbatim, and catalog %/* normalization is retained. Unit + e2e coverage is solid. One medium doc-contract inconsistency: get_tables/tables() describe catalog_name as a "pattern" whose empty value "matches nothing," while get_schemas/get_columns (identical handling) describe it as exact-or-all with empty preserved.
| cursor: The cursor object that will handle the results | ||
| catalog_name: Optional catalog name pattern to filter by | ||
| if catalog_name is None, we fetch across all catalogs | ||
| if catalog_name is None, we fetch across all catalogs; an empty |
There was a problem hiding this comment.
🟡 Medium — The catalog_name docstring for get_tables is inconsistent with the same argument on get_schemas and get_columns, even though all three route catalog_name through the identical _catalog_or_none helper (client.py:924/951/981).
get_schemas(L257) andget_columns(L337): "Optional exact catalog name.Noneleaves the filter unset;%and*select all catalogs; an empty string is preserved."get_tables(L295): "Optional catalog name pattern to filter by / if catalog_name is None, we fetch across all catalogs; an empty string matches nothing."
Since catalog handling is the same in all three (_catalog_or_none maps only None/%/* → unset and passes an empty string through verbatim), the get_tables wording is wrong on two points: it calls the catalog a "pattern" (it's exact-or-all) and claims an empty catalog "matches nothing" (it's actually preserved/passed through, matching the other two). The same divergence exists in the public docstrings in client.py: tables() says "Names are patterns... empty patterns match nothing" (implying catalog too), while schemas()/columns() say catalog is exact with %/* selecting all. Recommend aligning the get_tables/tables() catalog description with get_schemas/get_columns so callers aren't misled about catalog-filter semantics.
(Anchored to the nearest changed line — see the description for the exact location.)
1 participant
Fixes PECOBLR-4221.
The kernel metadata adapter no longer collapses empty or whitespace-only filters to
None. Empty pattern filters therefore match nothing, while exact filters retain the kernel's validation behavior. Existing%/*catalog wildcard normalization is unchanged.Testing: 265 focused unit tests passed;
git diff --checkpassed.