fix(csharp): metadata(csharp): empty-string catalog/schema/table arg throws HiveServer2Exception on Thrift, returns on SEA (#524)#577
Conversation
![peco-engineer-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
peco-engineer-bot
Bot
added
engineer-bot
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, correct parity fix. HasEmptyStringIdentifier properly treats null as "no filter" and only short-circuits on non-null zero-length strings, and the guards are placed before each base RPC across the five overrides. Two low-severity notes: the GetColumns guard also checks ColumnName (beyond issue scope, unverified against SEA), and the sibling GetColumnsExtendedAsync override was left unguarded despite the "uniform" claim in the knowledge log.
Addresses: - #3543375469 at csharp/src/DatabricksStatement.cs:869
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a small, well-contained parity fix. The HasEmptyStringIdentifier helper correctly treats only non-null zero-length strings as empty (null passes through as "no filter"), and each Thrift metadata override short-circuits to the same empty-result helpers already used elsewhere, before the base RPC that would throw. One Low note on CI-runnable unit coverage for the new pure helper.
Addresses: - #3543448305 at csharp/src/MetadataUtilities.cs:44
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, conservative parity fix. Empty-string identifier args now short-circuit to an empty result before hitting the Thrift RPC, matching SEA; the HasEmptyStringIdentifier helper (null = no filter, only zero-length triggers) is correct and well unit-tested, and the guards are placed before HandleSparkCatalog so empty catalog is caught. One low note: the GetCrossReferenceAsForeignTable guard lacks a dedicated E2E test.
Addresses: - #3543504258 at csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs:492
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested fix — the new HasEmptyStringIdentifier guard correctly short-circuits GetTables/GetColumns/GetPrimaryKeys/GetCrossReference(+ AsForeignTable) before the base RPC, and the null-vs-empty distinction is right. One medium parity gap: GetColumnsExtendedAsync's primary DESC path is unguarded, so empty catalog/schema still return real columns instead of empty (only empty-table is covered via the base fallback).
Addresses: - #3543665730 at csharp/src/DatabricksStatement.cs:1047
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-tested parity fix — HasEmptyStringIdentifier correctly treats only non-null zero-length strings as empty (null passes through), and the guards run before the base HiveServer2 RPCs that would throw. One low-severity note: the GetColumnsExtendedAsync guard is outside the scope described in the PR/learning.md and may introduce a Thrift↔SEA divergence for empty catalog/schema rather than remove one — worth confirming SEA's GetColumnsExtended returns empty for the same inputs. The five repeated guard blocks are near-identical boilerplate (nit — could be a small helper, not blocking).
Addresses: - #3543721922 at csharp/src/DatabricksStatement.cs:1091
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested parity fix. The new HasEmptyStringIdentifier helper is correct (non-null zero-length only; null passes through as "no filter"), and each guard returns an empty result whose schema matches the method's existing empty path. One low note: the GetColumnsExtended guard turns a previously-succeeding empty-catalog/empty-schema call into an empty result (a deliberate parity change with GetColumnsAsync, not an exception fix) — worth confirming that contract. Minor observation not worth a separate finding: GetSchemasAsync is the one metadata override left unguarded, which is consistent with issue #524's reported outcome diffs (list_schemas was not among them), so likely intentional.
| // On the primary DESC TABLE EXTENDED path, BuildTableName drops an empty catalog/schema, so | ||
| // an empty catalog or schema would otherwise produce a valid identifier and return real rows, | ||
| // diverging from GetColumnsAsync. Guard here so both column APIs behave identically. | ||
| if (MetadataUtilities.HasEmptyStringIdentifier(CatalogName, SchemaName, TableName)) |
There was a problem hiding this comment.
🔵 Low — This guard changes GetColumnsExtended behavior for the empty-catalog / empty-schema cases (with a non-empty table) from returning real rows to returning an empty result. Unlike GetTables/GetPrimaryKeys/GetCrossReference — where issue #524's actual defect was a thrown HiveServer2Exception — the DESC TABLE EXTENDED path did not throw here: BuildTableName() drops an empty catalog/schema and would resolve `schema`.`table` (or the default namespace) and return columns. The PR (correctly) makes this match GetColumnsAsync, which already returns empty for these args, so this is a deliberate parity change rather than a pure exception fix. Flagging only so a reviewer confirms the intent: callers that previously passed catalog="" expecting "use current catalog" resolution will now silently get zero rows. The change is documented in the inline comment and covered by tests, so no code change is required if the parity semantics are the desired contract.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
This thread asks a human reviewer to confirm that the empty-string→empty-result parity semantics (matching GetColumnsAsync / SEA) are the intended public contract for GetColumnsExtended, accepting that catalog=""/schema="" with a non-empty table no longer resolves to the current namespace but returns zero rows. The code is intentional, documented inline, and test-covered, so there is nothing to change here; the remaining decision is a semantic-contract judgment (is "empty string means no filter → empty result" the desired behavior, vs. "empty string means use current catalog") that needs human sign-off rather than further bot discussion.
Summary
Automated fix for #524 — metadata(csharp): empty-string catalog/schema/table arg throws HiveServer2Exception on Thrift, returns on SEA.
Added MetadataUtilities.HasEmptyStringIdentifier and short-circuited each Thrift metadata override (GetTables/GetColumns/GetPrimaryKeys/GetCrossReference/GetCrossReferenceAsForeignTable) in DatabricksStatement.cs to return an empty result set when any identifier arg is an empty string, before calling the base HiveServer2 RPC — matching SEA so empty-string args no longer throw HiveServer2Exception. All 8 E2E tests pass against the live endpoint (0 failed, 8 passed).
Files changed
csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cscsharp/src/MetadataUtilities.cscsharp/src/DatabricksStatement.cs.claude/knowledge/learning.mdTest plan
AdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetColumns_EmptyCatalog_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetColumns_EmptySchema_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetTables_EmptyCatalog_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetTables_EmptySchema_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetPrimaryKeys_EmptySchema_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetPrimaryKeys_EmptyTable_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetCrossReference_EmptyForeignSchema_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixAdbcDrivers.Databricks.Tests.E2E.StatementExecution.EmptyStringMetadataArgE2ETest.GetCrossReference_EmptyForeignTable_ReturnsEmptyWithoutThrowing— fails (red) against the original code, passes (green) after the fixFixes #524
🤖 Generated by engineer-bot (bug-fix flow)