GH-47709: [C++][FlightRPC] Return ODBC and data source information by alinaliBQ · Pull Request #47762 · apache/arrow

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

alinaliBQ · 2025-10-08T23:44:47Z

Rationale for this change

ODBC needs to give BI tools information about the driver itself and the data source it is connected to.

What changes are included in this PR?

Implementation of SQLGetinfo to return driver and data source information
Add default values for SQLGetInfo in get_info_cache.cc
Tests

Are these changes tested?

Tested on local MSVC Windows

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLGetInfo implementation #47709

alinaliBQ · 2025-10-08T23:45:06Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

github-actions · 2025-10-08T23:45:10Z

⚠️ GitHub issue #47709has been automatically assigned in GitHub to PR creator.

lidavidm · 2025-10-09T00:03:30Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


Could the connect/disconnect be put in a SetUp/TearDown?
Also it might not be wise to add the tests directly to FlightSQLODBCTestBase; could we create a fixture for this test suite specifically (by subclassing)?

yes, added separate test fixture ConnectionInfoTest

kou · 2025-10-09T06:37:54Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


Could you use static_cast?

alinaliBQ

Worked on code review comments

alinaliBQ · 2025-10-20T23:21:52Z

+// Driver Information
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoActiveEnvironments) {
+ this->Connect();


yes, added separate test fixture ConnectionInfoTest

alinaliBQ · 2025-10-20T23:32:53Z

+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetInfoDriverName) {
+ this->Connect();
+
+ Validate(this->conn, SQL_DRIVER_NAME, (SQLWCHAR*)L"Arrow Flight ODBC Driver");


lidavidm

Please rebase!

lidavidm · 2025-10-31T01:33:52Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


I do have the same general comment: I think you would be better off creating utility getters (that throw if they fail) and then using standard GTest assertions, instead of making a function for every combination of data type and comparator. (Also, can they be shared across test files?)

Yea when I saw your comment on the SQLStmtAttr PR, I thought of this PR as well since it also has the GreaterThan functions. I was waiting to see if the changes we made for SQLStmtAttr PR #47773 are what you are looking for. This is something we will be working on for sure.
On the other note, I am not sure if sharing certain utility getter across test files would be useful, since for example SQLGetInfo will only be called in connection_info_test.cc and no other test files. We won't be having copies of utility getters in different test files. 👍

Ah, if this is the same test file then never mind that.

@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-11-08T00:30:10Z

+void ValidateGreaterThan(SQLHDBC connection, SQLUSMALLINT info_type,
+ SQLULEN compared_value) {


@lidavidm I have fixed and added the utility getters, please have another look, thanks!

alinaliBQ · 2025-11-29T00:36:15Z

Rebased and tested PR changes on local MSVC. PR is ready to be merged

Co-Authored-By: rscales <robscales@icloud.com> Create connection_info_test.cc Add SQLGetInfo tests Co-authored-by: justing-bq <justin.gossett@improving.com> Co-Authored-By: rscales <robscales@icloud.com> Address code review comments Co-authored-by: justing-bq <justin.gossett@improving.com> Work on code review comments Fix build issues with tests Use Utility getters for SQLGetInfo Remove `using List` * fix build issues Add truncation test

conbench-apache-arrow · 2025-12-21T02:09:50Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2571c2c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709

github-actionsBot added Component: C++ Component: FlightRPC awaiting review Awaiting review labels Oct 8, 2025

lidavidm reviewed Oct 9, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 9, 2025

kou reviewed Oct 9, 2025
View reviewed changes

alinaliBQ mentioned this pull request Oct 10, 2025
GH-46734: [C++] Arrow Flight SQL ODBC layer #46099
Open

alinaliBQ force-pushed the gh-47709-sql-get-info branch from e6f61c6 to 49745a6Compare October 20, 2025 23:33

alinaliBQ commented Oct 20, 2025
View reviewed changes

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 20, 2025

lidavidm reviewed Oct 21, 2025
View reviewed changes

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

github-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Oct 21, 2025

lidavidm reviewed Oct 22, 2025
View reviewed changes

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

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 22, 2025

alinaliBQ force-pushed the gh-47709-sql-get-info branch from 5452bfd to f7a7386Compare October 22, 2025 18:50

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 22, 2025

alinaliBQ requested a review from lidavidm October 22, 2025 23:08

lidavidm reviewed Oct 31, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 31, 2025

justing-bq reviewed Nov 5, 2025
View reviewed changes

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

alinaliBQ mentioned this pull request Nov 7, 2025
Develop utility getters for ODBC APIs Bit-Quill/arrow#136
Merged

alinaliBQ force-pushed the gh-47709-sql-get-info branch from f7a7386 to f4aec24Compare November 8, 2025 00:29

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 8, 2025

alinaliBQ commented Nov 8, 2025
View reviewed changes

kou changed the title ~~GH-47709 : [C++][FlightRPC] Return ODBC and data source information~~ GH-47709: [C++][FlightRPC] Return ODBC and data source information Nov 9, 2025

lidavidm approved these changes Nov 10, 2025
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 10, 2025

alinaliBQ mentioned this pull request Nov 11, 2025
GH-47707: [C++][FlightRPC] Add tests for descriptor handle allocation #48053
Merged

alinaliBQ force-pushed the gh-47709-sql-get-info branch 4 times, most recently from 605971d to fc711aeCompare November 20, 2025 22:12

alinaliBQ force-pushed the gh-47709-sql-get-info branch from fc711ae to c701849Compare November 29, 2025 00:35

alinaliBQ marked this pull request as ready for review November 29, 2025 00:36

alinaliBQ force-pushed the gh-47709-sql-get-info branch from c701849 to 026b2a2Compare December 1, 2025 19:11

alinaliBQ requested a review from lidavidm December 5, 2025 20:04

jduo approved these changes Dec 8, 2025
View reviewed changes

jduo merged commit 2571c2c into apache:mainDec 8, 2025
45 checks passed

jduo mentioned this pull request Dec 8, 2025
[C++][FlightRPC][ODBC] SQLGetInfo implementation #47709
Closed