-
Notifications
You must be signed in to change notification settings - Fork 148
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
ca54b22
4be471e
cbcdcc5
c0da0bc
89b8999
a929fd1
0f8a432
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 |
|---|---|---|
| @@ -1 +1 @@ | ||
| 628abd6f5045897efcadb38ec77a1e9e0c23544e | ||
| d64009eb59404c1b082cb020296337f96dc0d4d7 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,8 +254,11 @@ def get_schemas( | |
| max_rows: Maximum number of rows to fetch in a single batch | ||
| max_bytes: Maximum number of bytes to fetch in a single batch | ||
| cursor: The cursor object that will handle the results | ||
| 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 | ||
| string is preserved. | ||
| schema_name: Optional schema name pattern to filter by. ``None`` | ||
| leaves the filter unset; an empty string matches nothing. | ||
|
|
||
| Returns: | ||
| ResultSet: An object containing the schema metadata | ||
|
|
@@ -290,10 +293,13 @@ def get_tables( | |
| max_bytes: Maximum number of bytes to fetch in a single batch | ||
| 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 | ||
|
vuanhphung marked this conversation as resolved.
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. 🔵 Low — The Catalog is not treated as a LIKE pattern (it flows through 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. 🔵 Low — The All three methods apply the same (Anchored to the nearest changed line — see the description for the exact location.) 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
Since catalog handling is the same in all three ( (Anchored to the nearest changed line — see the description for the exact location.) |
||
| string matches nothing | ||
| schema_name: Optional schema name pattern to filter by | ||
| if schema_name is None, we fetch across all schemas | ||
| table_name: Optional table name pattern to filter by | ||
| if schema_name is None, we fetch across all schemas; an empty | ||
| string matches nothing | ||
| table_name: Optional table name pattern to filter by. ``None`` | ||
| leaves the filter unset; an empty string matches nothing. | ||
| table_types: Optional list of table types to filter by (e.g., ['TABLE', 'VIEW']) | ||
|
|
||
| Returns: | ||
|
|
@@ -328,11 +334,16 @@ def get_columns( | |
| max_rows: Maximum number of rows to fetch in a single batch | ||
| max_bytes: Maximum number of bytes to fetch in a single batch | ||
| cursor: The cursor object that will handle the results | ||
| 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 | ||
| string is preserved. | ||
| schema_name: Optional schema name pattern to filter by. ``None`` | ||
| leaves the filter unset; an empty string matches nothing. | ||
| table_name: Optional table name pattern to filter by | ||
| if table_name is None, we fetch across all tables | ||
| column_name: Optional column name pattern to filter by | ||
| if table_name is None, we fetch across all tables; an empty | ||
| string matches nothing | ||
| column_name: Optional column name pattern to filter by. ``None`` | ||
| leaves the filter unset; an empty string matches nothing. | ||
|
|
||
| Returns: | ||
| ResultSet: An object containing the column metadata | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,35 +124,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 | ||
|
|
||
|
|
@@ -948,7 +922,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.)
vuanhphung marked this conversation as resolved.
|
||
| ) | ||
| return self._make_result_set(stream, cursor, self._synthetic_command_id()) | ||
| except Exception as exc: | ||
|
|
@@ -975,8 +949,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()) | ||
|
|
@@ -1005,9 +979,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.