-
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
e323e0e
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,35 +117,9 @@ 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 ("%", "*"): | ||
| """Map supported all-catalog wildcards to the kernel's unset filter.""" | ||
| if value is None or value in ("%", "*"): | ||
| return None | ||
| return value | ||
|
|
||
|
|
@@ -938,7 +912,7 @@ def get_schemas( | |
| try: | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — The change now forwards empty-string pattern filters ( But the previous helper's own docstring documented the opposite kernel behavior: The unit tests ( (Anchored to the nearest changed line — see the description for the exact location.) |
||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
| except Exception as exc: | ||
|
|
@@ -965,8 +939,8 @@ def get_tables( | |
| # 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), | ||
| schema_pattern=schema_name, | ||
|
vuanhphung marked this conversation as resolved.
|
||
| table_pattern=table_name, | ||
| table_types=table_types if table_types else None, | ||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
|
|
@@ -995,9 +969,9 @@ def get_columns( | |
| # 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), | ||
| 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 — This docstring lives on the abstract
DatabricksClientbase class, which is the shared contract for both the Thrift and kernel backends. It now statescatalog_nameis an "Optional exact catalog name" where "%and*select all catalogs." That semantics is kernel-only: the Thrift backend passescatalogName=catalog_namestraight through (thrift_backend.py:1160/1206/1254) with no%/*normalization, so on Thrift%is a literal catalog name — as the removed_catalog_or_nonecomment 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 publicCursordocstrings at client.py:1583-1584 and 1641-1642, which are likewise backend-agnostic and user-facing.