Skip to content

fix(csharp): metadata(csharp): empty-string catalog/schema/table arg throws HiveServer2Exception on Thrift, returns on SEA (#524) #577

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.

Sign up for GitHub

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

Open
peco-engineer-bot wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(csharp): metadata(csharp): empty-string catalog/schema/table arg throws HiveServer2Exception on Thrift, returns on SEA (#524) #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a1c34
fix(csharp): address issue #524
Jul 8, 2026
302deca
fix(csharp): address issue #577 (1 review thread)
peco-engineer-bot[bot] Jul 8, 2026
082a7f4
fix(csharp): address issue #577 (1 review thread)
peco-engineer-bot[bot] Jul 8, 2026
5f26464
fix(csharp): address issue #577 (1 review thread)
peco-engineer-bot[bot] Jul 8, 2026
71efaa2
fix(csharp): address issue #577 (1 review thread)
peco-engineer-bot[bot] Jul 8, 2026
9b70364
fix(csharp): address issue #577 (1 review thread)
peco-engineer-bot[bot] Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .claude/knowledge/learning.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ reader can grep for it.

## Entries

(no entries yet)
### 2026-07-08: metadata (Thrift vs SEA) — empty-string identifier arg must map to empty result, not an exception

**Issue:** #524 — passing an empty-string identifier (catalog="", schema="", table="", foreign_schema="", foreign_table="") to a metadata op threw `HiveServer2Exception` on the Thrift path but returned an empty `QueryResult` on SEA (1,763 outcome diffs across list_columns/list_tables/get_primary_keys/get_cross_reference).
**Root Cause:** `HiveServer2Statement.SetOption` stores the empty string verbatim into `CatalogName`/`SchemaName`/`TableName`/`Foreign*` and the Thrift metadata RPCs pass it straight through; the server rejects `""` (e.g. `ListTableSummaries` "name \"\" is not a valid name", GetPrimaryKeys "Can not create a Path from an empty string"). SEA already short-circuited via its `string.IsNullOrEmpty(...)` checks. The existing Thrift guards only covered `catalog != null` (multi-catalog) and PK/FK invalid-catalog — none caught a non-null empty string on schema/table/foreign args. `HandleSparkCatalog("")` returns `""` (only "SPARK"→null), so empty catalog also slipped through.
**Fix:** Added `MetadataUtilities.HasEmptyStringIdentifier(params string?[])` (true only for a non-null zero-length string; null = "no filter" untouched) and guarded the Databricks Thrift overrides in `DatabricksStatement.cs` (GetTables/GetColumns/GetPrimaryKeys/GetCrossReference/GetCrossReferenceAsForeignTable) to return the empty result *before* calling `base.*`, mirroring SEA. Note: on Thrift, GetColumns with catalog=""/schema="" already returned empty for some param combos (SHOW COLUMNS tolerates it) — only GetTables/PK/FK actually threw — but guarding all ops uniformly is the parity-correct behavior.
**Rule:** Normalize empty-string metadata identifier args to an empty result set up front in the editable Databricks overrides — never let `""` reach a base HiveServer2 RPC (it throws); null means "no filter" and must pass through.
79 changes: 79 additions & 0 deletions csharp/src/DatabricksStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,18 @@ protected override async Task<QueryResult> GetTablesAsync(CancellationToken canc
activity?.SetTag("statement.table_types", TableTypes ?? "(none)");
activity?.SetTag("statement.feature.enable_multiple_catalog_support", enableMultipleCatalogSupport);

// Issue #524: empty-string identifier -> empty result (parity with SEA);
// the Thrift RPC would otherwise raise HiveServer2Exception.
if (MetadataUtilities.HasEmptyStringIdentifier(CatalogName, SchemaName, TableName))
{
activity?.AddEvent("statement.get_tables.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_tables.complete");
return MetadataSchemaFactory.CreateEmptyTablesResult();
}

// Handle SPARK catalog case
HandleSparkCatalog();
activity?.SetTag("statement.catalog_name_after_spark_handling", CatalogName ?? "(none)");
Expand Down Expand Up @@ -852,6 +864,19 @@ protected override async Task<QueryResult> GetColumnsAsync(CancellationToken can
activity?.SetTag("statement.column_name", ColumnName ?? "(none)");
activity?.SetTag("statement.feature.enable_multiple_catalog_support", enableMultipleCatalogSupport);

// Issue #524: empty-string identifier -> empty result (parity with SEA);
// the Thrift RPC would otherwise raise HiveServer2Exception.
if (MetadataUtilities.HasEmptyStringIdentifier(CatalogName, SchemaName, TableName))
{
activity?.AddEvent("statement.get_columns.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_columns.complete");
return new QueryResult(0, new HiveInfoArrowStream(
MetadataSchemaFactory.CreateColumnMetadataSchema(), CreateColumnMetadataEmptyArray()));
}

// Handle SPARK catalog case
HandleSparkCatalog();
activity?.SetTag("statement.catalog_name_after_spark_handling", CatalogName ?? "(none)");
Expand Down Expand Up @@ -916,6 +941,18 @@ protected override async Task<QueryResult> GetPrimaryKeysAsync(CancellationToken
activity?.SetTag("statement.table_name", TableName ?? "(none)");
activity?.SetTag("statement.feature.pk_fk_enabled", enablePKFK);

// Issue #524: empty-string identifier -> empty result (parity with SEA);
// the Thrift RPC would otherwise raise HiveServer2Exception.
if (MetadataUtilities.HasEmptyStringIdentifier(CatalogName, SchemaName, TableName))
{
activity?.AddEvent("statement.get_primary_keys.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_primary_keys.complete");
return EmptyPrimaryKeysResult();
}

if (ShouldReturnEmptyPkFkResult())
{
activity?.AddEvent("statement.get_primary_keys.returning_empty_result", [
Expand Down Expand Up @@ -953,6 +990,20 @@ protected override async Task<QueryResult> GetCrossReferenceAsync(CancellationTo
activity?.SetTag("statement.foreign_table_name", ForeignTableName ?? "(none)");
activity?.SetTag("statement.feature.pk_fk_enabled", enablePKFK);

// Issue #524: empty-string identifier -> empty result (parity with SEA);
// the Thrift RPC would otherwise raise HiveServer2Exception.
if (MetadataUtilities.HasEmptyStringIdentifier(
CatalogName, SchemaName, TableName,
ForeignCatalogName, ForeignSchemaName, ForeignTableName))
{
activity?.AddEvent("statement.get_cross_reference.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_cross_reference.complete");
return EmptyCrossReferenceResult();
}

if (ShouldReturnEmptyPkFkResult())
{
activity?.AddEvent("statement.get_cross_reference.returning_empty_result", [
Expand Down Expand Up @@ -980,6 +1031,20 @@ protected override async Task<QueryResult> GetCrossReferenceAsForeignTableAsync(
activity?.SetTag("statement.foreign_catalog_name", ForeignCatalogName ?? "(none)");
activity?.SetTag("statement.feature.pk_fk_enabled", enablePKFK);

// Issue #524: empty-string identifier -> empty result (parity with SEA);
// the Thrift RPC would otherwise raise HiveServer2Exception.
if (MetadataUtilities.HasEmptyStringIdentifier(
CatalogName, SchemaName, TableName,
ForeignCatalogName, ForeignSchemaName, ForeignTableName))
{
activity?.AddEvent("statement.get_cross_reference_as_foreign_table.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_cross_reference_as_foreign_table.complete");
return EmptyCrossReferenceResult();
}

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
if (ShouldReturnEmptyPkFkResult())
{
activity?.AddEvent("statement.get_cross_reference_as_foreign_table.returning_empty_result", [
Expand Down Expand Up @@ -1018,6 +1083,20 @@ protected override async Task<QueryResult> GetColumnsExtendedAsync(CancellationT
activity?.SetTag("statement.desc_table_extended.full_table_name", fullTableName ?? "(none)");
activity?.SetTag("statement.desc_table_extended.can_use", canUseDescTableExtended);

// Issue #524: empty-string identifier -> empty result (parity with SEA and GetColumnsAsync).
// 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))

@peco-review-bot peco-review-bot Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@peco-engineer-bot peco-engineer-bot Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔ 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.

{
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
activity?.AddEvent("statement.get_columns_extended.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, 0);
activity?.AddEvent("statement.get_columns_extended.complete");
return CreateEmptyExtendedColumnsResult(MetadataSchemaFactory.CreateColumnMetadataSchema());
}

if (!canUseDescTableExtended || string.IsNullOrEmpty(fullTableName))
{
activity?.AddEvent("statement.get_columns_extended.fallback_to_base", [
Expand Down
21 changes: 21 additions & 0 deletions csharp/src/MetadataUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,27 @@ internal static class MetadataUtilities
return catalogName;
}

/// <summary>
/// Issue #524: an empty-string identifier argument (catalog="", schema="",
/// table="", foreign_*="") is a degenerate filter that can never match a real
/// object. The Thrift metadata RPCs reject it with a HiveServer2Exception,
/// while the SEA path returns an empty result set. To keep the two protocols
/// consistent (empty-string → empty result set, never an exception), callers
/// short-circuit to an empty result when any relevant identifier is empty.
///
/// Only a non-null, zero-length string counts; null means "no filter"
/// (e.g. "all catalogs") and is left untouched.
/// </summary>
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
internal static bool HasEmptyStringIdentifier(params string?[] identifiers)
{
foreach (string? identifier in identifiers)
{
if (identifier != null && identifier.Length == 0)
return true;
}
return false;
}

internal static bool IsInvalidPKFKCatalog(string? catalogName)
{
return string.IsNullOrEmpty(catalogName) ||
Expand Down
12 changes: 12 additions & 0 deletions csharp/src/StatementExecution/StatementExecutionStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1374,6 +1374,18 @@ private async Task<QueryResult> GetColumnsExtendedAsync(CancellationToken cancel
activity?.SetTag("table", _metadataTableName ?? "(none)");
activity?.SetTag("use_desc_table_extended", _connection.UseDescTableExtended);

// Issue #524: empty-string identifier -> empty result (parity with the Thrift
// DatabricksStatement.GetColumnsExtendedAsync guard). Without this, an empty
// catalog/schema with a non-empty table would be dropped by BuildQualifiedTableName,
// producing a valid identifier and real rows on the DESC path -> Thrift<->SEA divergence.
if (MetadataUtilities.HasEmptyStringIdentifier(_metadataCatalogName, _metadataSchemaName, _metadataTableName))
{
activity?.AddEvent("statement.get_columns_extended.returning_empty_result", [
new("reason", "Empty-string identifier argument")
]);
return CreateEmptyExtendedColumnsResult(MetadataSchemaFactory.CreateColumnMetadataSchema());
}

if (string.IsNullOrEmpty(_metadataTableName))
throw new ArgumentException("Table name is required for GetColumnsExtended");

Expand Down
Loading
Loading