Fix Thrift getFunctions catalog metadata - #536
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Focused, well-tested fix — the undefined sentinel cleanly scopes the override to getFunctions, and the row === null guard preserves disableRowMaterialization placeholders correctly. One low-severity question: the override unconditionally rewrites FUNCTION_CAT on every row, forcing null for omitted catalogs and the literal pattern for wildcards, which the PR itself notes wasn't re-verified live.
| return rows; | ||
| } | ||
|
|
||
| // Native GetFunctions leaves FUNCTION_CAT empty; match JDBC and kernel. |
There was a problem hiding this comment.
🔵 Low — This overwrites FUNCTION_CAT on every returned row with the literal requested catalogName, including two cases that discard/mislabel server data:
- Omitted catalog (
catalogName === undefined→functionCatalog === null): every row'sFUNCTION_CATis forced tonull, throwing away the actual catalog names the server returned. A caller doinggetFunctions({ functionName: '%' })across all catalogs now loses that column entirely. - Wildcard/pattern catalog (e.g.
catalogName: '%'): every row is stamped with the literal string'%', which is not a real catalog name — so rows that legitimately span multiple catalogs are all mislabeled.
The tests assert exactly this behavior, so it is intentional and claimed to match JDBC/kernel — but JDBC's getFunctions catalog argument is an exact name (not a pattern), whereas the Thrift catalogName here can be a pattern. Since the PR notes the live comparator was not rerun, please confirm against a live warehouse that stamping null/pattern values (rather than passing rows through) is the intended parity behavior for the omitted-catalog and wildcard cases.
|
Node.js integration tests triggered. View workflow runs. The result posts back here as the "Node.js Integration Tests" check. |
Override native Thrift
getFunctions()rows'FUNCTION_CATwith the original requested catalog, matching kernel and JDBC. Covers Arrow and column-based results, including prefetched data and omitted catalogs; other operations and empty-catalog validation are unchanged.Adds 35 regression tests. All 1,349 unit tests pass with inherited agent markers cleared; library/test type-checks, targeted ESLint, Prettier, and diff checks pass. The standard type-check still fails on existing example self-imports. Live comparator not rerun.
This PR was created with GitHub MCP.