GH-49482: [C++][FlightRPC][ODBC] Fix inconsistent SQLGetInfo values in global connection by vanshaj2023 · Pull Request #50021 · apache/arrow

vanshaj2023 · 2026-05-22T19:04:01Z

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: [C++][FlightRPC][ODBC] Fix inconsistent values in SQLGetInfo in global connection #49482

lidavidm

Thanks for the PR!

Also CC @alinaliBQ

lidavidm · 2026-05-25T06:34:58Z

+              bool supported =
+                  reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;


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

lidavidm · 2026-05-25T06:35:34Z

It seems there's some failing tests - some more test cases need updating?

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Resolves inconsistent return values for several ODBC connection info properties (SQL_CATALOG_LOCATION, SQL_DROP_SCHEMA, SQL_DROP_TABLE) by properly deriving them from Flight SQL boolean info responses, and replaces unchecked reinterpret_cast with checked_cast for scalar conversions.

Changes:

Use scalar boolean values from SQL_DDL_SCHEMA and SQL_DDL_TABLE to determine SQL_DROP_SCHEMA, SQL_CREATE_SCHEMA, SQL_DROP_TABLE, and SQL_CREATE_TABLE.
Avoid overwriting SQL_CATALOG_LOCATION when previously set, by using SetDefaultIfMissing.
Replace reinterpret_cast<BooleanScalar*> with checked_cast<BooleanScalar*> and update affected tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc	Derives DDL-related info values from server-provided booleans, guards `SQL_CATALOG_LOCATION` against overwrite, and switches to `checked_cast`.
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc	Updates tests to reflect corrected behavior, moving some tests from `ConnectionInfoHandleTest` to `ConnectionInfoTest`/`ConnectionInfoMockTest`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

vanshaj2023 · 2026-05-25T14:54:48Z

Hi @lidavidm,
Thanks for the review! I've addressed both comments switched to checked_cast and updated the DropSchema/DropTable test expectations to SQL_DS_DROP_SCHEMA/SQL_DT_DROP_TABLE since the mock sends true for both DDL flags. CI failures are a pre-existing flake unrelated to this change.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

alinaliBQ

Hi @vanshaj2023 thanks for raising the PR. Please build and test ODBC locally first.

FYI - Once these 2 PRs are merged, ODBC will be tested against a Dremio instance in the Linux CI. This might be helpful.

alinaliBQ · 2026-05-25T17:29:51Z

@vanshaj2023 To ensure maximum coverage, please build and run the ODBC tests locally against a Dremio docker instance with documentation here:

arrow/cpp/src/arrow/flight/sql/odbc/README.md

Lines 88 to 89 in cd1811b

## Steps to Run the ODBC tests

After ODBC has been registered, you can run the ODBC tests. It is recommended to run the ODBC tests locally first.

alinaliBQ · 2026-05-25T17:31:42Z

              transactions_supported =
-                  reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;


Please fix the CI build failures from checked_cast in this file:
https://github.com/apache/arrow/actions/runs/26406439067/job/77730895486?pr=50021#step:7:4419

/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc:499:19: error: use of undeclared identifier 'checked_cast'; did you mean 'internal::checked_cast'? 499 | checked_cast<BooleanScalar*>(scalar->child_value().get())->value; | ^~~~~~~~~~~~ | internal::checked_cast /Users/runner/work/arrow/arrow/cpp/src/arrow/util/checked_cast.h:28:19: note: 'internal::checked_cast' declared here

alinaliBQ · 2026-05-25T21:31:33Z

@justing-bq please review future updates.

vanshaj2023 · 2026-05-26T12:08:30Z

Thanks @alinaliBQ! Fixed both issues
qualified checked_cast with using arrow::internal::checked_cast; to match the convention used elsewhere in flight/sql, and switched the DropSchema/DropTable tests back to TYPED_TEST(ConnectionInfoTest, ...). I'll run the tests against a Dremio Docker instance and report back.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

alinaliBQ · 2026-05-27T23:48:23Z

@lidavidm Folks with maintainer access might need to trigger the C++ Extra CI checks in this PR, could you have a look at the CI? From my end, I can only see: Label when reviewed / Label PRs when reviewed (pull_request_review), not sure why.

I see at https://github.com/apache/arrow/actions/workflows/cpp_extra.yml?query=branch%3Afix%2FGH-49482-fix-sqlgetinfo-inconsistent-values, the C++ Extra CI wasn't triggered for the newest commit 8ac407a.

@vanshaj2023 Sounds good. How are the local Dremio instance test results? And maybe we could do a new push to get the C++ Extra CIs to run?

manyiResearch · 2026-07-01T09:44:19Z

Hi, just a quick question — since this PR also seems related to #49500, does it resolve that issue too? Thanks!

lidavidm · 2026-07-01T22:19:30Z

@vanshaj2023 can you rebase this PR?

alinaliBQ · 2026-08-05T23:03:23Z

There are 2 failures in C++ Extra / ODBC Linux against the dremio instance:

[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ConnectionInfoTest/1.TestSQLGetInfoDropTable, where TypeParam = arrow::flight::sql::odbc::FlightSQLODBCRemoteTestBase
[  FAILED  ] ConnectionInfoRemoteTest.TestSQLGetInfoDropSchema

 2 FAILED TESTS

And could you fix the Dev / Lint C++ failure as well?

Dremio does not report table or schema DDL support any more than the SQLite mock does, so split TestSQLGetInfoDropTable per backend too.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/src/arrow/flight/sql/odbc/odbc_impl/get_info_cache.cc:482

SetDefaultIfMissing() uses unordered_map::emplace(), which will never overwrite an existing key. Since info_ already contains SQL_CATALOG_LOCATION from the constructor (and is also defaulted in LoadDefaultsForMissingEntries()), this block will never update the value based on SQL_CATALOG_AT_START, so it doesn't actually serve as a fallback and may not resolve ordering-dependent behavior as intended.

If the goal is “use SQL_CATALOG_AT_START only when ARROW_SQL_CATALOG_TERM didn’t set the location”, consider tracking a local catalog_location_set/saw_catalog_term flag during LoadInfoFromServer() and conditionally assigning info_[SQL_CATALOG_LOCATION] instead of relying on emplace() (or remove the eager initialization of SQL_CATALOG_LOCATION so it can truly be set here).

              SetDefaultIfMissing(
                  info_, SQL_CATALOG_LOCATION,
                  static_cast<uint16_t>(
                      checked_cast<BooleanScalar*>(scalar->child_value().get())->value
                          ? SQL_CL_START

alinaliBQ

I have one comment for TestSQLGetInfoDropSchema, other comments are out of scope for this PR but helpful info

alinaliBQ · 2026-08-06T16:22:45Z

  GetInfo(this->conn, SQL_DROP_TABLE, &value);

+  // The Dremio backend does not report table DDL support.
  EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);


Out of scope of this PR but helpful to mention, there is a gap that 2 tests are missing from the original implementation:

TEST_F(ConnectionInfoRemoteTest, TestSQLGetInfoCreateTable) for SQL_CREATE_TABLE

TEST_F(ConnectionInfoRemoteTest, TestSQLGetInfoCreateSchema) for SQL_CREATE_SCHEMA
Theoretically these tests should also return 0 to signal table and schema DDL is not supported in Dremio instance. It also explains why we were only seeing inconsistencies in SQL_DROP_TABLE and SQL_DROP_SCHEMA.

I raised #50828 for this

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

alinaliBQ · 2026-08-06T17:24:51Z

+TEST_F(ConnectionInfoRemoteTest, TestSQLGetInfoDropSchema) {
+  SQLUINTEGER value;
+  GetInfo(this->conn, SQL_DROP_SCHEMA, &value);
+
+  // The Dremio backend does not report schema DDL support either.
+  EXPECT_EQ(static_cast<SQLUINTEGER>(0), value);


can be removed

alinaliBQ · 2026-08-06T17:25:10Z

  SQLUINTEGER value;
  GetInfo(this->conn, SQL_DROP_SCHEMA, &value);

+  // SQLite (the mock backend) does not support schema DDL.


we can change the comment to mention both SQLite and Dremio

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

vanshaj2023 · 2026-08-06T17:32:37Z

@alinaliBQ thanks for the suggestions and for catching my typo-fix. Pushed 870e231:

Removed the now-duplicate ConnectionInfoRemoteTest.TestSQLGetInfoDropSchema since TYPED_TEST(ConnectionInfoHandleTest, TestSQLGetInfoDropSchema) already covers both backends.
Updated the comment to mention both SQLite and Dremio.

Left TestSQLGetInfoDropTable as separate mock/remote TEST_Fs since those two backends genuinely disagree on the value (mock expects SQL_DT_DROP_TABLE, remote expects 0), so it can't be consolidated the same way.

Also noted #50828 for the missing remote CreateTable/CreateSchema coverage, thanks for filing that separately.

alinaliBQ · 2026-08-06T20:24:58Z

The code change looks good, pending ODBC CI workflow re-run: https://github.com/apache/arrow/actions/runs/31123496079?pr=50021
Looks like the CI failed due to connection issue.

lidavidm · 2026-08-06T23:06:34Z

GitHub had an outage. May want to rebase, I tried restarting the jobs but they did not run.

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

alinaliBQ

+1. LGTM, the CI failures look unrelated.

alinaliBQ · 2026-08-07T17:44:40Z

Just a heads-up that after this PR, I probably won't have much bandwidth for additional reviews due to some other commitments. Thanks! cc @lidavidm

vanshaj2023 · 2026-08-07T18:48:36Z

Thanks @alinaliBQ really appreciate the careful review and your helpful suggestions which helps me to fix the issue...

alinaliBQ · 2026-08-07T19:49:21Z

@vanshaj2023 You're welcome! Glad that I was able to help

vanshaj2023 requested a review from lidavidm as a code owner May 22, 2026 19:04

github-actions Bot added Component: C++ Component: FlightRPC CI: Extra: C++ Run extra C++ CI awaiting review Awaiting review labels May 22, 2026

lidavidm reviewed May 25, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 25, 2026

Copilot AI lite review requested due to automatic review settings May 25, 2026 14:50

Copilot AI reviewed May 25, 2026

View reviewed changes

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 25, 2026

vanshaj2023 requested review from Copilot and lidavidm May 25, 2026 14:56

Copilot AI reviewed May 25, 2026

View reviewed changes

alinaliBQ reviewed May 25, 2026

View reviewed changes

vanshaj2023 requested a review from Copilot May 26, 2026 12:08

Copilot AI reviewed May 26, 2026

View reviewed changes

vanshaj2023 requested a review from alinaliBQ May 27, 2026 20:58

vanshaj2023 added 3 commits August 3, 2026 03:49

apacheGH-49482: fix inconsistent SQLGetInfo values in global connection

4b13ca4

address review: use checked_cast and fix test expectations for DDL info

1864e21

address review: qualify checked_cast and use TYPED_TEST for DDL tests

7a68a25

vanshaj2023 force-pushed the fix/GH-49482-fix-sqlgetinfo-inconsistent-values branch from 8ac407a to 7a68a25 Compare August 2, 2026 22:20

Copilot AI review requested due to automatic review settings August 2, 2026 22:20

fix clang-format and correct DropTable/DropSchema remote expectations

de092ed

Dremio does not report table or schema DDL support any more than the SQLite mock does, so split TestSQLGetInfoDropTable per backend too.

Copilot AI review requested due to automatic review settings August 6, 2026 06:24

Copilot started reviewing on behalf of vanshaj2023 August 6, 2026 06:24 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

alinaliBQ reviewed Aug 6, 2026

View reviewed changes

Update cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc

cfb3d10

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 6, 2026 17:22

Copilot AI reviewed Aug 6, 2026

vanshaj2023 requested a review from alinaliBQ August 6, 2026 17:24

alinaliBQ reviewed Aug 6, 2026

View reviewed changes

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated

alinaliBQ reviewed Aug 6, 2026

View reviewed changes

Update cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc

cbb6d27

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 6, 2026 17:25

Copilot AI reviewed Aug 6, 2026

address review: remove duplicate DropSchema remote test, fix comment

870e231

Copilot AI review requested due to automatic review settings August 6, 2026 17:31

Copilot AI reviewed Aug 6, 2026

vanshaj2023 requested a review from alinaliBQ August 6, 2026 17:33

alinaliBQ reviewed Aug 7, 2026

View reviewed changes

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated

Update cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc

9e47bdf

Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 7, 2026 16:52

Copilot AI reviewed Aug 7, 2026

alinaliBQ approved these changes Aug 7, 2026

View reviewed changes

		bool supported =
		reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;

	## Steps to Run the ODBC tests
	After ODBC has been registered, you can run the ODBC tests. It is recommended to run the ODBC tests locally first.

		transactions_supported =
		reinterpret_cast<BooleanScalar*>(scalar->child_value().get())->value;

Uh oh!

Conversation

vanshaj2023 commented May 22, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

How to test

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

vanshaj2023 commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

alinaliBQ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinaliBQ commented May 25, 2026

Uh oh!

vanshaj2023 commented May 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

alinaliBQ commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manyiResearch commented Jul 1, 2026

Uh oh!

lidavidm commented Jul 1, 2026

Uh oh!

alinaliBQ commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

alinaliBQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

vanshaj2023 commented Aug 6, 2026

Uh oh!

alinaliBQ commented Aug 6, 2026

vanshaj2023 commented May 22, 2026 •

edited by github-actions Bot

Loading

vanshaj2023 commented May 25, 2026 •

edited

Loading

alinaliBQ left a comment •

edited

Loading

alinaliBQ commented May 27, 2026 •

edited

Loading