GH-49482: [C++][FlightRPC][ODBC] Fix inconsistent SQLGetInfo values in global connection

Project: apache/arrow | Language: C++ | Concepts: error-handling, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/apache/arrow/pull/50021

The problem

The ODBC driver was returning inconsistent values for certain SQLGetInfo properties (like SQL_DROP_SCHEMA, SQL_CREATE_SCHEMA, SQL_DROP_TABLE, SQL_CREATE_TABLE, and SQL_CATALOG_LOCATION) when using a global (reused) connection. These inconsistencies depended on the order in which properties were loaded and whether the backend supported certain DDL features, causing tests to fail or pass unpredictably.

Why it's non-trivial

A naive fix might simply set these info values to a default or hardcoded value. However, this would not correctly reflect backend capabilities (e.g., whether schema or table DDL are actually supported), and would not resolve the subtle bug where pre-seeded defaults could prevent the correct value from being loaded from the server. Additionally, some info values depend on others, so the order and method of setting them matters for correctness and test repeatability.

The approach

1. Use the backend's reported support for DDL to set related info values, instead of hardcoding:

-  info_[SQL_DROP_SCHEMA] = static_cast<uint32_t>(0);
-  info_[SQL_DROP_TABLE] = static_cast<uint32_t>(0);

Removed pre-seeded defaults for SQL_DROP_SCHEMA and SQL_DROP_TABLE from the constructor, so their values are only set based on actual backend support.

2. Set SQL_DROP_SCHEMA and SQL_CREATE_SCHEMA based on the boolean value from the backend:

+              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);

If the backend reports schema DDL support, set the appropriate bitmask; otherwise, set to zero.

3. Apply the same logic for table DDL support:

+              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);

Ensures SQL_CREATE_TABLE and SQL_DROP_TABLE reflect backend capabilities.

4. Fix ordering-dependent conflict for SQL_CATALOG_LOCATION:

-              info_[SQL_CATALOG_LOCATION] = static_cast<uint16_t>(
-                  reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value
-                      ? SQL_CL_START
-                      : SQL_CL_END);
+              SetDefaultIfMissing(
+                  info_, SQL_CATALOG_LOCATION,
+                  static_cast<uint16_t>(
+                      checked_cast<BooleanScalar*>(scalar->child_value().get())->value
+                          ? SQL_CL_START
+                          : SQL_CL_END));

Uses SetDefaultIfMissing so that SQL_CATALOG_LOCATION is only set if not already set by another property, avoiding conflicts.

5. Use checked_cast for type safety when accessing scalar values:

-  return reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value ? "Y" : "N";
+  return checked_cast<BooleanScalar*>(scalar->child_value().get())->value ? "Y" : "N";

Replaces unsafe casts with a safer, explicit checked cast.

6. Refactor and fix tests to match new logic and backend differences:

-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);
 }

Removes unnecessary connect/disconnect and moves test to the correct fixture.

-  // 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);
 }

Test now expects 0 for both backends, matching the new logic.

From the review thread

lidavidm:
Maybe we can use checked_cast from arrow/util/checked_cast.h?

alinaliBQ:
@vanshaj2023 thanks for catching those, found the actual causes:

1. SQL_DROP_SCHEMA/SQL_DROP_TABLE were pre-seeded to 0 in the constructor, which short-circuited GetInfo's load-on-miss check — the real value only loaded if some other property triggered it first. Removed the premature defaults.
2. The SQLite mock genuinely has no schema DDL support (SQL_DDL_SCHEMA = false), so the Create/DropSchema mock expectations were just wrong. Fixed to expect 0 for mock, split DropSchema into mock/remote since support differs by backend.

Takeaways

• Avoid hardcoding or pre-seeding values that should depend on runtime or backend state; always defer to the authoritative source when possible.
• When multiple properties are interdependent or can be set in different orders, use conditional or fallback setters to prevent conflicts and ensure consistent results.
• Prefer type-safe casting utilities (like checked_cast) over raw casts to prevent subtle bugs and clarify intent.
• Tests should accurately reflect the real capabilities of each backend, and avoid workarounds that mask underlying inconsistencies.