diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 301f77d9..1eae488a 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -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. diff --git a/csharp/src/DatabricksStatement.cs b/csharp/src/DatabricksStatement.cs index bd7135fe..f61be4a6 100644 --- a/csharp/src/DatabricksStatement.cs +++ b/csharp/src/DatabricksStatement.cs @@ -806,6 +806,18 @@ protected override async Task 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)"); @@ -852,6 +864,19 @@ protected override async Task 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)"); @@ -916,6 +941,18 @@ protected override async Task 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", [ @@ -953,6 +990,20 @@ protected override async Task 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", [ @@ -980,6 +1031,20 @@ protected override async Task 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(); + } + if (ShouldReturnEmptyPkFkResult()) { activity?.AddEvent("statement.get_cross_reference_as_foreign_table.returning_empty_result", [ @@ -1018,6 +1083,20 @@ protected override async Task 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)) + { + 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", [ diff --git a/csharp/src/MetadataUtilities.cs b/csharp/src/MetadataUtilities.cs index 9391abf3..70323425 100644 --- a/csharp/src/MetadataUtilities.cs +++ b/csharp/src/MetadataUtilities.cs @@ -31,6 +31,27 @@ internal static class MetadataUtilities return catalogName; } + /// + /// 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. + /// + 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) || diff --git a/csharp/src/StatementExecution/StatementExecutionStatement.cs b/csharp/src/StatementExecution/StatementExecutionStatement.cs index 72e2ecbc..eaf6f958 100644 --- a/csharp/src/StatementExecution/StatementExecutionStatement.cs +++ b/csharp/src/StatementExecution/StatementExecutionStatement.cs @@ -1374,6 +1374,18 @@ private async Task 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"); diff --git a/csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs b/csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs index 3173654e..e1c571fb 100644 --- a/csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs +++ b/csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs @@ -330,4 +330,256 @@ public void GetInfo_ReturnsDriverInfo() Assert.True(batch!.Length >= 5, "GetInfo should return at least 5 info codes"); } } + + /// + /// Issue #524: an empty-string identifier argument (catalog="", schema="", + /// table="", foreign_schema="", foreign_table="") passed to a metadata op must + /// NOT throw HiveServer2Exception on the Thrift path. It must return an empty + /// result set, matching the SEA/REST path (Thrift vs SEA outcome parity). + /// + /// These tests force the Thrift protocol (adbc.databricks.protocol=thrift) so the + /// regression reproduces regardless of the CI run's default protocol; the Thrift + /// path is where the divergence lives. + /// + public class EmptyStringMetadataArgE2ETest : TestBase + { + public EmptyStringMetadataArgE2ETest(ITestOutputHelper? outputHelper) + : base(outputHelper, new DatabricksTestEnvironment.Factory()) + { + } + + private void SkipIfNotConfigured() + { + Skip.IfNot(Utils.CanExecuteTestConfig(TestConfigVariable), "Test configuration not available"); + } + + // Connection pinned to the Thrift protocol regardless of the configured default. + private AdbcConnection CreateThriftConnection() + { + var parameters = GetDriverParameters(TestConfiguration); + parameters[DatabricksParameters.Protocol] = "thrift"; + var driver = new DatabricksDriver(); + var db = driver.Open(parameters); + return db.Connect(new Dictionary()); + } + + // Connection pinned to the SEA (REST) protocol regardless of the configured default. + private AdbcConnection CreateSeaConnection() + { + var parameters = GetDriverParameters(TestConfiguration); + parameters[DatabricksParameters.Protocol] = "rest"; + var driver = new DatabricksDriver(); + var db = driver.Open(parameters); + return db.Connect(new Dictionary()); + } + + /// + /// Executes a metadata command and returns the total row count. Must not throw. + /// + private static async Task ExecuteMetadataRowCount(AdbcConnection connection, string command, + string? catalog = null, string? schema = null, string? table = null, + string? foreignCatalog = null, string? foreignSchema = null, string? foreignTable = null) + { + using var stmt = connection.CreateStatement(); + stmt.SetOption(ApacheParameters.IsMetadataCommand, "true"); + if (catalog != null) stmt.SetOption(ApacheParameters.CatalogName, catalog); + if (schema != null) stmt.SetOption(ApacheParameters.SchemaName, schema); + if (table != null) stmt.SetOption(ApacheParameters.TableName, table); + if (foreignCatalog != null) stmt.SetOption(ApacheParameters.ForeignCatalogName, foreignCatalog); + if (foreignSchema != null) stmt.SetOption(ApacheParameters.ForeignSchemaName, foreignSchema); + if (foreignTable != null) stmt.SetOption(ApacheParameters.ForeignTableName, foreignTable); + + stmt.SqlQuery = command; + QueryResult result = await stmt.ExecuteQueryAsync(); + + int rows = 0; + using var reader = result.Stream!; + while (true) + { + using var batch = await reader.ReadNextRecordBatchAsync(); + if (batch == null) break; + rows += batch.Length; + } + return rows; + } + + // GetColumns + + [SkippableFact] + public async Task GetColumns_EmptyCatalog_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumns", + catalog: "", schema: TestConfiguration.Metadata.Schema, table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetColumns_EmptySchema_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumns", + catalog: TestConfiguration.Metadata.Catalog, schema: "", table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + // GetTables + + [SkippableFact] + public async Task GetTables_EmptyCatalog_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetTables", + catalog: "", schema: TestConfiguration.Metadata.Schema); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetTables_EmptySchema_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetTables", + catalog: TestConfiguration.Metadata.Catalog, schema: ""); + Assert.Equal(0, rows); + } + + // GetPrimaryKeys + + [SkippableFact] + public async Task GetPrimaryKeys_EmptySchema_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetPrimaryKeys", + catalog: TestConfiguration.Metadata.Catalog, schema: "", table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetPrimaryKeys_EmptyTable_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetPrimaryKeys", + catalog: TestConfiguration.Metadata.Catalog, schema: TestConfiguration.Metadata.Schema, table: ""); + Assert.Equal(0, rows); + } + + // GetCrossReference + + [SkippableFact] + public async Task GetCrossReference_EmptyForeignSchema_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetCrossReference", + catalog: TestConfiguration.Metadata.Catalog, + schema: TestConfiguration.Metadata.Schema, + table: TestConfiguration.Metadata.Table, + foreignCatalog: TestConfiguration.Metadata.Catalog, + foreignSchema: "", + foreignTable: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetCrossReference_EmptyForeignTable_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetCrossReference", + catalog: TestConfiguration.Metadata.Catalog, + schema: TestConfiguration.Metadata.Schema, + table: TestConfiguration.Metadata.Table, + foreignCatalog: TestConfiguration.Metadata.Catalog, + foreignSchema: TestConfiguration.Metadata.Schema, + foreignTable: ""); + Assert.Equal(0, rows); + } + + // GetColumnsExtended + // + // GetCrossReferenceAsForeignTableAsync has no metadata command name of its own; + // it is reached only through the base GetColumnsExtendedAsync, which the Databricks + // override delegates to on its fallback path (BuildTableName empty => empty TableName). + // On that path the base helper invokes GetCrossReferenceAsForeignTableAsync with the + // statement's own catalog/schema/table bound to the FOREIGN slot, so an empty-string + // identifier there is what the new guard (DatabricksStatement.cs ~L1031) must absorb. + // Without the guard the foreign-table cross-reference RPC would raise + // HiveServer2Exception. Empty table forces the fallback, exercising that guard. + + [SkippableFact] + public async Task GetColumnsExtended_EmptyTable_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumnsExtended", + catalog: TestConfiguration.Metadata.Catalog, + schema: TestConfiguration.Metadata.Schema, + table: ""); + Assert.Equal(0, rows); + } + + // On the primary DESC TABLE EXTENDED path, BuildTableName drops an empty catalog/schema, + // so without the guard these would produce a valid identifier and return real rows, + // diverging from GetColumnsAsync. These cover the empty-catalog / empty-schema cases with + // a non-empty table (which do NOT force the empty-table fallback). + + [SkippableFact] + public async Task GetColumnsExtended_EmptyCatalog_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumnsExtended", + catalog: "", + schema: TestConfiguration.Metadata.Schema, + table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetColumnsExtended_EmptySchema_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateThriftConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumnsExtended", + catalog: TestConfiguration.Metadata.Catalog, + schema: "", + table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + // SEA-path parity: the empty-catalog / empty-schema short-circuit lives in + // StatementExecutionStatement.GetColumnsExtendedAsync as well. Without it, the DESC + // path's BuildQualifiedTableName drops the empty part and returns real rows, diverging + // from Thrift. These SEA-pinned variants verify both protocols agree on empty results. + + [SkippableFact] + public async Task GetColumnsExtended_EmptyCatalog_Sea_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateSeaConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumnsExtended", + catalog: "", + schema: TestConfiguration.Metadata.Schema, + table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + + [SkippableFact] + public async Task GetColumnsExtended_EmptySchema_Sea_ReturnsEmptyWithoutThrowing() + { + SkipIfNotConfigured(); + using var conn = CreateSeaConnection(); + int rows = await ExecuteMetadataRowCount(conn, "GetColumnsExtended", + catalog: TestConfiguration.Metadata.Catalog, + schema: "", + table: TestConfiguration.Metadata.Table); + Assert.Equal(0, rows); + } + } } diff --git a/csharp/test/Unit/MetadataUtilitiesTests.cs b/csharp/test/Unit/MetadataUtilitiesTests.cs index e8869b20..f762b255 100644 --- a/csharp/test/Unit/MetadataUtilitiesTests.cs +++ b/csharp/test/Unit/MetadataUtilitiesTests.cs @@ -136,5 +136,49 @@ public void BuildQualifiedTableName_BackticksEscaped() { Assert.Equal("`main`.`my``schema`.`my``table`", MetadataUtilities.BuildQualifiedTableName("main", "my`schema", "my`table")); } + + // HasEmptyStringIdentifier + + [Fact] + public void HasEmptyStringIdentifier_EmptyString_ReturnsTrue() + { + Assert.True(MetadataUtilities.HasEmptyStringIdentifier("")); + } + + [Fact] + public void HasEmptyStringIdentifier_Null_ReturnsFalse() + { + Assert.False(MetadataUtilities.HasEmptyStringIdentifier((string?)null)); + } + + [Fact] + public void HasEmptyStringIdentifier_Whitespace_ReturnsFalse() + { + Assert.False(MetadataUtilities.HasEmptyStringIdentifier(" ")); + } + + [Fact] + public void HasEmptyStringIdentifier_NonEmpty_ReturnsFalse() + { + Assert.False(MetadataUtilities.HasEmptyStringIdentifier("main")); + } + + [Fact] + public void HasEmptyStringIdentifier_NoArgs_ReturnsFalse() + { + Assert.False(MetadataUtilities.HasEmptyStringIdentifier()); + } + + [Fact] + public void HasEmptyStringIdentifier_MixedWithEmpty_ReturnsTrue() + { + Assert.True(MetadataUtilities.HasEmptyStringIdentifier("main", null, "", "users")); + } + + [Fact] + public void HasEmptyStringIdentifier_MixedNoEmpty_ReturnsFalse() + { + Assert.False(MetadataUtilities.HasEmptyStringIdentifier("main", null, "default", "users")); + } } }