fix(kernel): preserve empty metadata filters - #933
Conversation
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."
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.
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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Focused, well-tested fix — removing _none_if_blank so empty/whitespace metadata filters reach the kernel unchanged is clean, and unit + e2e coverage matches the new semantics. One medium concern: the public Cursor docstrings (shared by the Thrift and kernel backends) now assert kernel-specific %/* and empty-string semantics unconditionally, which the Thrift path doesn't implement.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, intentional behavior change that stops collapsing empty/whitespace metadata pattern filters to None, with matching unit + e2e coverage and an updated CHANGELOG. One low-severity note about an untested empty-catalog edge on get_columns.
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.