-
Notifications
You must be signed in to change notification settings - Fork 147
fix(kernel): preserve empty metadata filters #933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
46e620a
74d883f
765a95b
96c3ba0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,39 +117,6 @@ def _is_not_found(exc: BaseException) -> bool: | |
| ) | ||
|
|
||
|
|
||
| def _none_if_blank(value: Optional[str]) -> Optional[str]: | ||
| """Map an empty/whitespace-only metadata filter to ``None`` | ||
| ("match all"), matching the Thrift backend's effective behaviour. | ||
|
|
||
| The kernel's ``Identifier`` / ``LikePattern`` reject ``""`` with | ||
| ``InvalidArgument`` (-> ``ProgrammingError``); ``None`` is the | ||
| kernel's canonical "match all". Applied to schema / table / column | ||
| *pattern* args (which otherwise keep ``%`` / ``_`` as real LIKE | ||
| wildcards).""" | ||
| if value is None: | ||
| return None | ||
| return value if value.strip() else None | ||
|
|
||
|
|
||
| def _catalog_or_none(value: Optional[str]) -> Optional[str]: | ||
| """Normalise a catalog filter: ``None`` / blank / ``'%'`` / ``'*'`` | ||
| all mean "all catalogs" -> ``None``. | ||
|
|
||
| This makes ``columns(catalog='%')`` behave like | ||
| ``tables(catalog='%')`` / ``schemas(catalog='%')`` — the kernel | ||
| already treats blank/``%``/``*`` as "all catalogs" for SHOW SCHEMAS | ||
| / SHOW TABLES (``is_null_or_wildcard``) but treats the catalog as an | ||
| exact identifier for SHOW COLUMNS, so the three diverged. Normalising | ||
| connector-side makes them symmetric. This intentionally diverges from | ||
| raw-Thrift literalness (Thrift treats ``%`` as a literal catalog | ||
| name) in favour of JDBC "catalog is exact-or-all, not a pattern" + | ||
| internal consistency. Catalog is the only arg normalised this way; | ||
| schema/table/column patterns keep ``%`` / ``*`` as LIKE wildcards.""" | ||
| if value is None or not value.strip() or value in ("%", "*"): | ||
| return None | ||
| return value | ||
|
|
||
|
|
||
| def _is_staging_statement(operation: str) -> bool: | ||
| """True iff ``operation`` is a volume/staging statement (PUT / GET / | ||
| REMOVE). | ||
|
|
@@ -937,8 +904,8 @@ def get_schemas( | |
| raise InterfaceError("get_schemas requires an open session.") | ||
| try: | ||
| stream = self._kernel_session.metadata().list_schemas( | ||
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| catalog=catalog_name, | ||
| schema_pattern=schema_name, | ||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
| except Exception as exc: | ||
|
|
@@ -964,9 +931,9 @@ def get_tables( | |
| # do the work — no connector-side drain + refilter. Passing it | ||
| # through preserves streaming for large schemas. | ||
| stream = self._kernel_session.metadata().list_tables( | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — The bridge exists specifically because the kernel does not match-nothing on an empty catalog. The docstring of the removed Either way, Suggest routing 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,
) |
||
| schema_pattern=schema_name, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium —
This leaves the empty-catalog semantics of The e2e (Anchored to the nearest changed line — see the description for the exact location.) |
||
| table_pattern=table_name, | ||
| table_types=table_types if table_types else None, | ||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
|
|
@@ -994,10 +961,10 @@ def get_columns( | |
| # Thrift backend's `getColumns(null, …)` behaviour from | ||
| # the user's perspective. | ||
| stream = self._kernel_session.metadata().list_columns( | ||
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| table_pattern=_none_if_blank(table_name), | ||
| column_pattern=_none_if_blank(column_name), | ||
| catalog=catalog_name, | ||
| schema_pattern=schema_name, | ||
| table_pattern=table_name, | ||
| column_pattern=column_name, | ||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
| except Exception as exc: | ||
|
|
||
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.
🔵 Low — The three metadata docstrings now describe
catalog_nameinconsistently after this change:get_schemas(L251) andget_columns(L330) were updated to "Optional exact catalog name to filter by, forwarded unchanged", butget_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 iftables()accepts a catalog pattern whileschemas()/columns()accept an exact name — a distinction a caller would reasonably act on. Either theget_tableswording should be aligned with the sibling methods, or (if the divergence is intentional because the kernel treats the catalog differently forSHOW TABLES/SHOW SCHEMASvsSHOW COLUMNS, as the now-removed_catalog_or_nonedocstring 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.)