This is an automated email from the ASF dual-hosted git repository.
curth pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new 241477e92 feat(csharp/src/Drivers/Databricks): Remove redundant
CloseOperation for GetColumnsAsync (#3132)
241477e92 is described below
commit 241477e922b7c1ef32ff9618bb698f09b5059f20
Author: Todd Meng <[email protected]>
AuthorDate: Fri Jul 11 06:29:49 2025 -0700
feat(csharp/src/Drivers/Databricks): Remove redundant CloseOperation for
GetColumnsAsync (#3132)
[A previous PR](https://github.com/apache/arrow-adbc/pull/3093) fixed
redundant CloseOperations that were throwing errors and causing failure
for dbr 11.3 and below.
GetColumnsAsync uses a seperate response handling path, which does not
call GetQueryResults (where the previous fix was implemented), so this
operation still fails. This PR adds a fix
---
.../Drivers/Apache/Hive2/HiveServer2Statement.cs | 3 +
.../test/Drivers/Databricks/E2E/StatementTests.cs | 81 ++++++++++++++++++----
2 files changed, 70 insertions(+), 14 deletions(-)
diff --git a/csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs
b/csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs
index d25ae4fb1..3eebb5773 100644
--- a/csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs
+++ b/csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs
@@ -523,6 +523,9 @@ namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2
cancellationToken);
OperationHandle = resp.OperationHandle;
+ // Set _directResults so that dispose logic can check if operation
was already closed
+ _directResults = resp.DirectResults;
+
// Common variables declared upfront
TGetResultSetMetadataResp metadata;
Schema schema;
diff --git a/csharp/test/Drivers/Databricks/E2E/StatementTests.cs
b/csharp/test/Drivers/Databricks/E2E/StatementTests.cs
index 3b67050d6..bc1e0e22a 100644
--- a/csharp/test/Drivers/Databricks/E2E/StatementTests.cs
+++ b/csharp/test/Drivers/Databricks/E2E/StatementTests.cs
@@ -133,25 +133,78 @@ namespace Apache.Arrow.Adbc.Tests.Drivers.Databricks
}
/// <summary>
- /// Verifies that Dispose() can be called on metadata query statements
without throwing
- /// "Invalid OperationHandle" errors. This tests the fix for the issue
where the server
- /// auto-closes operations but the client still tries to close them
during disposal.
+ /// Comprehensive test that verifies disposal works for all statement
types without throwing exceptions.
+ /// This prevents regressions like the GetColumns disposal bug where
_directResults wasn't set properly.
/// </summary>
- [SkippableFact]
- public async Task CanDisposeMetadataQueriesWithoutError()
+ [SkippableTheory]
+ [InlineData("ExecuteStatement", "SELECT 1 as test_column")]
+ [InlineData("GetCatalogs", "GetCatalogs")]
+ [InlineData("GetSchemas", "GetSchemas")]
+ [InlineData("GetTables", "GetTables")]
+ [InlineData("GetColumns", "GetColumns")]
+ [InlineData("GetPrimaryKeys", "GetPrimaryKeys")]
+ [InlineData("GetCrossReference", "GetCrossReference")]
+ [InlineData("GetColumnsExtended", "GetColumnsExtended")]
+ public async Task AllStatementTypesDisposeWithoutErrors(string
statementType, string sqlCommand)
{
- // Test a simple metadata command that's most likely to trigger
the issue
var statement = Connection.CreateStatement();
- statement.SetOption(ApacheParameters.IsMetadataCommand, "true");
- statement.SqlQuery = "GetSchemas";
- // Execute the metadata query
- QueryResult queryResult = await statement.ExecuteQueryAsync();
- Assert.NotNull(queryResult.Stream);
+ try
+ {
+ if (statementType == "ExecuteStatement")
+ {
+ // Regular SQL statement
+ statement.SqlQuery = sqlCommand;
+ }
+ else
+ {
+ // Metadata command
+ statement.SetOption(ApacheParameters.IsMetadataCommand,
"true");
+ statement.SqlQuery = sqlCommand;
+
+ // Set required parameters for specific metadata commands
+ if (sqlCommand is "GetColumns" or "GetPrimaryKeys" or
"GetCrossReference" or "GetColumnsExtended")
+ {
+ statement.SetOption(ApacheParameters.CatalogName,
TestConfiguration.Metadata.Catalog);
+ statement.SetOption(ApacheParameters.SchemaName,
TestConfiguration.Metadata.Schema);
+ statement.SetOption(ApacheParameters.TableName,
TestConfiguration.Metadata.Table);
+ }
+
+ if (sqlCommand == "GetCrossReference")
+ {
+ // GetCrossReference needs foreign table parameters too
+
statement.SetOption(ApacheParameters.ForeignCatalogName,
TestConfiguration.Metadata.Catalog);
+
statement.SetOption(ApacheParameters.ForeignSchemaName,
TestConfiguration.Metadata.Schema);
+ statement.SetOption(ApacheParameters.ForeignTableName,
TestConfiguration.Metadata.Table);
+ }
+ }
+
+ // Execute the statement
+ QueryResult queryResult = await statement.ExecuteQueryAsync();
+ Assert.NotNull(queryResult.Stream);
- // This should not throw "Invalid OperationHandle" errors
- // The fix ensures _directResults is set so dispose logic works
correctly
- statement.Dispose();
+ // Consume at least one batch to ensure the operation completes
+ var batch = await
queryResult.Stream.ReadNextRecordBatchAsync();
+ // Note: batch might be null for empty results, that's OK
+
+ // The critical test: disposal should not throw any exceptions
+ // This specifically tests the fix for the GetColumns bug
where _directResults wasn't set
+ var exception = Record.Exception(() => statement.Dispose());
+ Assert.Null(exception);
+ }
+ catch (Exception ex)
+ {
+ // If execution fails, we still want to test disposal
+ OutputHelper?.WriteLine($"Statement execution failed for
{statementType}: {ex.Message}");
+
+ // Even if execution failed, disposal should not throw
+ var disposalException = Record.Exception(() =>
statement.Dispose());
+ Assert.Null(disposalException);
+
+ // Re-throw the original exception if we want to investigate
execution failures
+ // For now, we'll skip the test if execution fails since
disposal is our main concern
+ Skip.If(true, $"Skipping disposal test for {statementType} due
to execution failure: {ex.Message}");
+ }
}
[SkippableFact]
