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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
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."
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| table_pattern=_none_if_blank(table_name), | ||
| schema_pattern=schema_name, |
There was a problem hiding this comment.
🟡 Medium — get_tables handles an empty catalog differently from get_schemas/get_columns, and no test locks its actual match-nothing behavior.
get_schemas and get_columns route the catalog through _exact_catalog_and_pattern, which special-cases catalog == "" → catalog=None, schema_pattern="" precisely because (per the new helper docstring) those kernel APIs take an exact Identifier that rejects "". get_tables instead passes catalog=_catalog_or_none(catalog_name), which returns "" verbatim for an empty string.
This leaves the empty-catalog semantics of tables(catalog_name="") resting entirely on how the kernel's list_tables interprets a blank catalog. The comment removed in this PR documented that the kernel treats blank/%/* as "all catalogs" for SHOW TABLES via is_null_or_wildcard (exact-identifier only for SHOW COLUMNS). If that is still true at KERNEL_REV, then tables(catalog_name="") would match every catalog — the opposite of the "empty string matches nothing" contract this PR adds to the docstrings/CHANGELOG, and inconsistent with schemas()/columns().
The e2e test_columns_with_empty_string_filter_matches_nothing and test_schemas_with_empty_string_filter_matches_nothing cover columns and schemas, and the unit test_get_tables_preserves_empty_patterns only asserts the pass-through call args (catalog="") — none verify that tables(catalog_name="") actually returns nothing. Please confirm the kernel's list_tables treats an empty catalog as match-nothing (not match-all); if it doesn't, get_tables needs the same _exact_catalog_and_pattern adaptation. Either way, an e2e case for tables(catalog_name="") would lock the intended semantics.
(Anchored to the nearest changed line — see the description for the exact location.)
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.
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| table_pattern=_none_if_blank(table_name), | ||
| catalog=catalog_name, |
There was a problem hiding this comment.
🟠 High — get_tables passes catalog=catalog_name directly to list_tables, bypassing the new _exact_catalog_and_pattern bridge that get_schemas and get_columns use for the empty-catalog case.
The bridge exists specifically because the kernel does not match-nothing on an empty catalog. The docstring of the removed _catalog_or_none (this PR's own diff) states the kernel "treats blank/%/* as 'all catalogs' for SHOW SCHEMAS / SHOW TABLES (is_null_or_wildcard)". By that same logic, list_tables(catalog="") will be interpreted as all catalogs — matching everything — rather than nothing. (If instead the kernel routes the tables catalog through an exact Identifier, "" raises InvalidArgument → ProgrammingError, which is exactly what the bridge was introduced to avoid.)
Either way, cursor.tables(catalog_name="") diverges from the stated contract and from the sibling methods: the docstring this PR adds to databricks_client.py (get_tables) reads "if catalog_name is None, we fetch across all catalogs; an empty string matches nothing", and the PR summary claims empty catalog/schema/table/column filters all "match nothing." get_tables with an empty catalog does not honor that.
Suggest routing get_tables through the same bridge:
catalog, schema_pattern = _exact_catalog_and_pattern(catalog_name, schema_name)
stream = self._kernel_session.metadata().list_tables(
catalog=catalog,
schema_pattern=schema_pattern,
table_pattern=table_name,
table_types=table_types if table_types else None,
)| list_schemas.assert_called_once_with(catalog=None, schema_pattern="") | ||
|
|
||
|
|
||
| def test_get_tables_preserves_empty_patterns(): |
There was a problem hiding this comment.
🔵 Low — Test coverage for the empty-catalog case is asymmetric with the code. get_schemas and get_columns each get a dedicated *_empty_catalog_uses_empty_pattern unit test asserting catalog=None, schema_pattern="", but test_get_tables_preserves_empty_patterns only asserts catalog="" is passed through verbatim — there is no test asserting tables(catalog_name="") matches nothing. Likewise the e2e test_columns_with_empty_string_filter_matches_nothing parametrizes all four column filters, and the schemas e2e only exercises schema_name="" (not catalog_name=""); no e2e locks empty-catalog match-nothing for tables(). This gap is why the F1 divergence isn't caught. Add an empty-catalog case for get_tables (unit + e2e) once F1's behavior is decided.
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.
| 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 three metadata docstrings now describe catalog_name inconsistently after this change: get_schemas (L251) and get_columns (L330) were updated to "Optional exact catalog name to filter by, forwarded unchanged", but get_tables (L288) was left as "Optional catalog name pattern to filter by". Since all three now forward the catalog verbatim to the kernel, the public contract reads as if tables() accepts a catalog pattern while schemas()/columns() accept an exact name — a distinction a caller would reasonably act on. Either the get_tables wording should be aligned with the sibling methods, or (if the divergence is intentional because the kernel treats the catalog differently for SHOW TABLES/SHOW SCHEMAS vs SHOW COLUMNS, as the now-removed _catalog_or_none docstring described) that per-method difference should be stated explicitly rather than left as an accidental wording mismatch.
(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 strings to
None; it forwards all filters unchanged, matching the Node.js kernel path.Noneremains the only unset filter, and empty pattern filters match nothing. Empty exact catalogs retain the kernel's current validation behavior.Testing: 265 focused unit tests passed;
git diff --checkpassed.