This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 77daf985d2d GH-49482: [C++][FlightRPC][ODBC] Fix inconsistent
SQLGetInfo values in global connection (#50021)
77daf985d2d is described below
commit 77daf985d2d3b9172e00042141d4ab512e5c1cdf
Author: Kumar Vanshaj <[email protected]>
AuthorDate: Mon Aug 10 04:50:41 2026 +0530
GH-49482: [C++][FlightRPC][ODBC] Fix inconsistent SQLGetInfo values in
global connection (#50021)
## What changed
- `SQL_DDL_SCHEMA` case now reads the boolean scalar to conditionally set
`SQL_DROP_SCHEMA` and `SQL_CREATE_SCHEMA` to `0` when unsupported, instead of
always writing non-zero values
- `SQL_DDL_TABLE` case applies the same pattern for `SQL_DROP_TABLE` and
`SQL_CREATE_TABLE`
- `SQL_CATALOG_AT_START` changed to use `SetDefaultIfMissing` so it does
not overwrite `SQL_CATALOG_LOCATION` already set by `ARROW_SQL_CATALOG_TERM`,
eliminating an ordering-dependent conflict
- Removed per-test connect/disconnect workarounds and converted three tests
from `ConnectionInfoHandleTest` back to `ConnectionInfoTest`
## How to test
- Build ODBC tests and run `TestSQLGetInfoCatalogLocation`,
`TestSQLGetInfoDropSchema`, `TestSQLGetInfoDropTable` with both mock server and
global connection fixture
- All three tests should pass consistently regardless of connection reuse
Closes #49482
* GitHub Issue: #49482
Lead-authored-by: vanshaj2023 <[email protected]>
Co-authored-by: vanshaj2023 <[email protected]>
Co-authored-by: Kumar Vanshaj <[email protected]>
Co-authored-by: Alina (Xi) Li <[email protected]>
Signed-off-by: David Li <[email protected]>
---
.../flight/sql/odbc/odbc_impl/get_info_cache.cc | 70 +++++++++++-----------
.../flight/sql/odbc/tests/connection_info_test.cc | 38 +++++-------
2 files changed, 50 insertions(+), 58 deletions(-)
diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc
b/cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc
index 6923d7fafbe..1d9e9ee4c18 100644
--- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc
+++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc
@@ -25,6 +25,7 @@
#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h"
#include "arrow/scalar.h"
#include "arrow/type_fwd.h"
+#include "arrow/util/checked_cast.h"
#include "arrow/flight/sql/odbc/odbc_impl/flight_sql_stream_chunk_buffer.h"
#include "arrow/flight/sql/odbc/odbc_impl/scalar_function_reporter.h"
@@ -76,6 +77,9 @@
#define ARROW_CONVERT_VARCHAR 19
namespace arrow::flight::sql::odbc {
+
+using arrow::internal::checked_cast;
+
namespace {
// Return the corresponding field in SQLGetInfo's SQL_CONVERT_* field
// types for the given Arrow SqlConvert enum value.
@@ -190,7 +194,7 @@ inline int64_t ScalarToInt64(UnionScalar* scalar) {
}
inline std::string ScalarToBoolString(UnionScalar* scalar) {
- return reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
? "Y" : "N";
+ return checked_cast<BooleanScalar*>(scalar->child_value().get())->value ?
"Y" : "N";
}
inline void SetDefaultIfMissing(std::unordered_map<uint16_t,
Connection::Info>& cache,
@@ -246,8 +250,6 @@ GetInfoCache::GetInfoCache(FlightClientOptions&
client_options,
info_[SQL_DROP_CHARACTER_SET] = static_cast<uint32_t>(0);
info_[SQL_DROP_COLLATION] = static_cast<uint32_t>(0);
info_[SQL_DROP_DOMAIN] = static_cast<uint32_t>(0);
- info_[SQL_DROP_SCHEMA] = static_cast<uint32_t>(0);
- info_[SQL_DROP_TABLE] = static_cast<uint32_t>(0);
info_[SQL_DROP_TRANSLATION] = static_cast<uint32_t>(0);
info_[SQL_DROP_VIEW] = static_cast<uint32_t>(0);
info_[SQL_MAX_IDENTIFIER_LEN] = static_cast<uint16_t>(65535); // arbitrary
@@ -395,25 +397,21 @@ bool GetInfoCache::LoadInfoFromServer() {
// Unused by ODBC.
break;
case SqlInfoOptions::SQL_DDL_SCHEMA: {
- // GH-49500 TODO: use scalar bool to determine
`SQL_CREATE_SCHEMA` and
- // `SQL_DROP_SCHEMA` values
-
- // Note: this is a bitmask and we can't describe cascade or
restrict
- // flags.
- info_[SQL_DROP_SCHEMA] =
static_cast<uint32_t>(SQL_DS_DROP_SCHEMA);
-
- // Note: this is a bitmask and we can't describe authorization or
- // collation
- info_[SQL_CREATE_SCHEMA] =
static_cast<uint32_t>(SQL_CS_CREATE_SCHEMA);
+ bool supported =
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
+ info_[SQL_DROP_SCHEMA] =
+ static_cast<uint32_t>(supported ? SQL_DS_DROP_SCHEMA : 0);
+ info_[SQL_CREATE_SCHEMA] =
+ static_cast<uint32_t>(supported ? SQL_CS_CREATE_SCHEMA : 0);
break;
}
case SqlInfoOptions::SQL_DDL_TABLE: {
- // GH-49500 TODO: use scalar bool to determine
`SQL_CREATE_TABLE` and
- // `SQL_DROP_TABLE` values
-
- // This is a bitmask and we cannot describe all clauses.
- info_[SQL_CREATE_TABLE] =
static_cast<uint32_t>(SQL_CT_CREATE_TABLE);
- info_[SQL_DROP_TABLE] = static_cast<uint32_t>(SQL_DT_DROP_TABLE);
+ bool supported =
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
+ info_[SQL_CREATE_TABLE] =
+ static_cast<uint32_t>(supported ? SQL_CT_CREATE_TABLE : 0);
+ info_[SQL_DROP_TABLE] =
+ static_cast<uint32_t>(supported ? SQL_DT_DROP_TABLE : 0);
break;
}
case SqlInfoOptions::SQL_ALL_TABLES_ARE_SELECTABLE: {
@@ -426,7 +424,7 @@ bool GetInfoCache::LoadInfoFromServer() {
}
case SqlInfoOptions::SQL_NULL_PLUS_NULL_IS_NULL: {
info_[SQL_CONCAT_NULL_BEHAVIOR] = static_cast<uint16_t>(
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value
? SQL_CB_NULL
: SQL_CB_NON_NULL);
break;
@@ -436,7 +434,7 @@ bool GetInfoCache::LoadInfoFromServer() {
// SQL_SUPPORTS_DIFFERENT_TABLE_CORRELATION_NAMES since we need
both
// properties to determine the value for SQL_CORRELATION_NAME.
supports_correlation_name =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
break;
}
case
SqlInfoOptions::SQL_SUPPORTS_DIFFERENT_TABLE_CORRELATION_NAMES: {
@@ -444,7 +442,7 @@ bool GetInfoCache::LoadInfoFromServer() {
// SQL_SUPPORTS_DIFFERENT_TABLE_CORRELATION_NAMES since we need
both
// properties to determine the value for SQL_CORRELATION_NAME.
requires_different_correlation_name =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
break;
}
case SqlInfoOptions::SQL_SUPPORTS_EXPRESSIONS_IN_ORDER_BY: {
@@ -454,9 +452,8 @@ bool GetInfoCache::LoadInfoFromServer() {
case SqlInfoOptions::SQL_SUPPORTS_ORDER_BY_UNRELATED: {
// Note: this is the negation of the Flight SQL property.
info_[SQL_ORDER_BY_COLUMNS_IN_SELECT] =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
- ? "N"
- : "Y";
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value ? "N"
+
: "Y";
break;
}
case SqlInfoOptions::SQL_SUPPORTS_LIKE_ESCAPE_CLAUSE: {
@@ -465,7 +462,7 @@ bool GetInfoCache::LoadInfoFromServer() {
}
case SqlInfoOptions::SQL_SUPPORTS_NON_NULLABLE_COLUMNS: {
info_[SQL_NON_NULLABLE_COLUMNS] = static_cast<uint16_t>(
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value
? SQL_NNC_NON_NULL
: SQL_NNC_NULL);
break;
@@ -475,10 +472,15 @@ bool GetInfoCache::LoadInfoFromServer() {
break;
}
case SqlInfoOptions::SQL_CATALOG_AT_START: {
- info_[SQL_CATALOG_LOCATION] = static_cast<uint16_t>(
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
- ? SQL_CL_START
- : SQL_CL_END);
+ // Only use this as a fallback if ARROW_SQL_CATALOG_TERM has not
already
+ // set SQL_CATALOG_LOCATION (to avoid conflicting writes
depending on
+ // response key ordering).
+ SetDefaultIfMissing(
+ info_, SQL_CATALOG_LOCATION,
+ static_cast<uint16_t>(
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value
+ ? SQL_CL_START
+ : SQL_CL_END));
break;
}
case SqlInfoOptions::SQL_SELECT_FOR_UPDATE_SUPPORTED:
@@ -494,22 +496,22 @@ bool GetInfoCache::LoadInfoFromServer() {
}
case SqlInfoOptions::SQL_TRANSACTIONS_SUPPORTED: {
transactions_supported =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
break;
}
case
SqlInfoOptions::SQL_DATA_DEFINITION_CAUSES_TRANSACTION_COMMIT: {
transaction_ddl_commit =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
break;
}
case SqlInfoOptions::SQL_DATA_DEFINITIONS_IN_TRANSACTIONS_IGNORED:
{
transaction_ddl_ignore =
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value;
break;
}
case SqlInfoOptions::SQL_BATCH_UPDATES_SUPPORTED: {
info_[SQL_BATCH_SUPPORT] = static_cast<uint32_t>(
-
reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
+
checked_cast<BooleanScalar*>(scalar->child_value().get())->value
? SQL_BS_ROW_COUNT_EXPLICIT
: 0);
break;
diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc
b/cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc
index 232aa985c7e..ff4abc59fa1 100644
--- a/cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc
+++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc
@@ -30,6 +30,7 @@ template <typename T>
class ConnectionInfoTest : public T {};
class ConnectionInfoMockTest : public FlightSQLODBCMockTestBase {};
+class ConnectionInfoRemoteTest : public FlightSQLODBCRemoteTestBase {};
using TestTypes = ::testing::Types<ConnectionInfoMockTest,
FlightSQLODBCRemoteTestBase>;
TYPED_TEST_SUITE(ConnectionInfoTest, TestTypes);
@@ -622,18 +623,11 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoAlterTable) {
EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);
}
-TYPED_TEST(ConnectionInfoHandleTest, TestSQLGetInfoCatalogLocation) {
- // GH-49482 TODO: resolve inconsitent return value for SQL_CATALOG_LOCATION
and change
- // test type to `ConnectionInfoTest`
- this->ConnectWithString(this->GetConnectionString(), this->conn);
-
+TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoCatalogLocation) {
SQLUSMALLINT value;
GetInfo(this->conn, SQL_CATALOG_LOCATION, &value);
EXPECT_EQ(static_cast<SQLUSMALLINT>(0), value);
-
- EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn))
- << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoCatalogName) {
@@ -706,7 +700,8 @@ TEST_F(ConnectionInfoMockTest, TestSQLGetInfoCreateSchema) {
SQLUINTEGER value;
GetInfo(this->conn, SQL_CREATE_SCHEMA, &value);
- EXPECT_EQ(static_cast<SQLUINTEGER>(1), value);
+ // SQLite (the mock backend) does not support schema DDL.
+ EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);
}
TEST_F(ConnectionInfoMockTest, TestSQLGetInfoCreateTable) {
@@ -758,32 +753,27 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDropDomain) {
EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);
}
-TYPED_TEST(ConnectionInfoHandleTest, TestSQLGetInfoDropSchema) {
- // GH-49482 TODO: resolve inconsitent return value for SQL_DROP_SCHEMA and
change test
- // type to `ConnectionInfoTest`
- this->ConnectWithString(this->GetConnectionString(), this->conn);
-
+TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDropSchema) {
SQLUINTEGER value;
GetInfo(this->conn, SQL_DROP_SCHEMA, &value);
+ // Neither the SQLite mock backend nor the Dremio backend support schema DDL.
EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);
-
- EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn))
- << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn);
}
-TYPED_TEST(ConnectionInfoHandleTest, TestSQLGetInfoDropTable) {
- // GH-49482 TODO: resolve inconsitent return value for SQL_DROP_TABLE and
change test
- // type to `ConnectionInfoTest`
- this->ConnectWithString(this->GetConnectionString(), this->conn);
+TEST_F(ConnectionInfoMockTest, TestSQLGetInfoDropTable) {
+ SQLUINTEGER value;
+ GetInfo(this->conn, SQL_DROP_TABLE, &value);
+ EXPECT_EQ(static_cast<SQLUINTEGER>(SQL_DT_DROP_TABLE), value);
+}
+
+TEST_F(ConnectionInfoRemoteTest, TestSQLGetInfoDropTable) {
SQLUINTEGER value;
GetInfo(this->conn, SQL_DROP_TABLE, &value);
+ // The Dremio backend does not report table DDL support.
EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);
-
- EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn))
- << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDropTranslation) {
