Skip to content

GH-47708: [C++][FlightRPC] Connection Attribute Support for ODBC #47772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

GH-47708: [C++][FlightRPC] Connection Attribute Support for ODBC #47772

wants to merge 3 commits into from
+406 −24

Conversation

alinaliBQ
Copy link
Contributor

@alinaliBQ alinaliBQ commented Oct 9, 2025 •
edited by github-actions bot
Loading

Rationale for this change

Add connection attribute support for ODBC driver.

What changes are included in this PR?

  • Implementation of SQLGetConnectAttr and SQLSetConnectAttr to get and set connection attributes
  • Tests

Are these changes tested?

Will be tested in CI when PR is ready for review

Are there any user-facing changes?

No

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2025

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

@alinaliBQ
Copy link
Contributor Author

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

@github-actions github-actions bot added the awaiting review Awaiting review label Oct 9, 2025
lidavidm
lidavidm reviewed Oct 10, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as other PRs:

  • We can create a separate test fixture
  • We can put Connect/Disconnect in SetUp/TearDown
  • We should probably be using ASSERT_ not EXPECT_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use ASSERT_ where applicable. Other items are in-progress. Connect/Disconnect have been removed from tests and we will move it to SetUp/TearDown

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 10, 2025
kou
kou reviewed Oct 10, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc Outdated

#ifdef SQL_ATTR_ASYNC_DBC_EVENT
TYPED_TEST(FlightSQLODBCTestBase, TestSQLSetConnectAttrAsyncDbcEventUnsupported) {
this->Connect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetUp() as David commented in other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this->Connect() from tests. we will move it to SetUp

@kou
Copy link
Member

kou commented Oct 10, 2025

Could you create new draft PRs after you complete other draft PRs to reflect comments in other draft PRs?

@alinaliBQ
Copy link
Contributor Author

@lidavidm @kou Sure, will check the general comments first before creating new PRs. I will put the PRs to a pause for now since the comments are similar. Thanks for letting me know

@alinaliBQ alinaliBQ mentioned this pull request Oct 10, 2025
Open
@alinaliBQ alinaliBQ force-pushed the gh-47708-sql-connect-attr branch from af5de2e to e8c404c Compare October 21, 2025 22:15
alinaliBQ
alinaliBQ commented Oct 21, 2025
View reviewed changes
Copy link
Contributor Author

@alinaliBQ alinaliBQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use ASSERT_ where applicable. Other items are in-progress. Connect/Disconnect have been removed from tests and we will move it to SetUp/TearDown

cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc Outdated

#ifdef SQL_ATTR_ASYNC_DBC_EVENT
TYPED_TEST(FlightSQLODBCTestBase, TestSQLSetConnectAttrAsyncDbcEventUnsupported) {
this->Connect();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this->Connect() from tests. we will move it to SetUp

@github-actions github-actions bot added awaiting review Awaiting review awaiting change review Awaiting change review awaiting committer review Awaiting committer review and removed awaiting changes Awaiting changes awaiting review Awaiting review awaiting change review Awaiting change review labels Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou left review comments

@lidavidm lidavidm lidavidm left review comments

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alinaliBQ @kou @lidavidm