Skip to content

GH-47711: [C++][FlightRPC] Enable ODBC query execution #48032

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
Merged
lidavidm merged 1 commit into from
Dec 4, 2025
Merged

GH-47711: [C++][FlightRPC] Enable ODBC query execution #48032

lidavidm merged 1 commit into from
Dec 4, 2025
+119 −11

Conversation

@alinaliBQ
Copy link
Contributor

@alinaliBQ alinaliBQ commented Nov 3, 2025 •
edited
Loading

Rationale for this change

Enable query execution in ODBC.

What changes are included in this PR?

  • Extract SQLExecDirect, SQLExecute, SQLPrepare implementation & tests

Are these changes tested?

  • Tested on local MSVC

Are there any user-facing changes?

N/A

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

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

alinaliBQ
alinaliBQ commented Nov 3, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/odbc_api.cc
Comment on lines +813 to +1016
statement->Prepare(query);
statement->ExecutePrepared();
Copy link
Contributor Author

@alinaliBQ alinaliBQ Nov 3, 2025

Choose a reason for hiding this comment

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

we implemented SQLExecDirect to use prepare/execute to use prepared statements to align with the behavior with JDBC

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 3, 2025
@alinaliBQ alinaliBQ force-pushed the gh-47711-query-execution branch from 72e8db0 to 97c94b0 Compare November 3, 2025 20:42
justing-bq
justing-bq reviewed Nov 6, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc
ASSERT_EQ(SQL_SUCCESS,
SQLExecDirect(this->stmt, &sql0[0], static_cast<SQLINTEGER>(sql0.size())));

// GH-47713 TODO: Uncomment call to SQLFetch SQLGetData after implementation
Copy link
Contributor

@justing-bq justing-bq Nov 6, 2025

Choose a reason for hiding this comment

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

I thought we weren't expecting these tests to pass yet? Are there build issues with this not commented out?

Copy link
Contributor Author

@alinaliBQ alinaliBQ Nov 7, 2025

Choose a reason for hiding this comment

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

When this PR is just created as draft, we aren't expecting them to pass. But once the pre-requisite ODBC API PRs get merged in main, I am expecting this PR to pass the CI tests after a rebase. And some calls to unimplemented API (SQLFetch and SQLGetData) are commented out because they will be enabled in a different PR

@alinaliBQ alinaliBQ changed the title GH-47711: [C++][FlightRPC] WIP: Enable ODBC query execution GH-47711: [C++][FlightRPC] Enable ODBC query execution Nov 10, 2025
alinaliBQ
alinaliBQ commented Nov 10, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_statement.cc
return boost::make_optional(it != attribute_.end(), it->second);
}

boost::optional<std::shared_ptr<ResultSetMetadata>> FlightSqlStatement::Prepare(
Copy link
Contributor Author

@alinaliBQ alinaliBQ Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Working on removing boost as part of #48067

Copy link
Contributor Author

@alinaliBQ alinaliBQ Dec 1, 2025

Choose a reason for hiding this comment

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

GitHub issue for removing boost::optional: #48084

@alinaliBQ alinaliBQ mentioned this pull request Nov 11, 2025
Open
@alinaliBQ
Copy link
Contributor Author

alinaliBQ commented Nov 12, 2025

@lidavidm this draft PR is ready for review! Please have a look

@alinaliBQ alinaliBQ force-pushed the gh-47711-query-execution branch from 97c94b0 to 8625cfa Compare November 15, 2025 00:06
@alinaliBQ alinaliBQ force-pushed the gh-47711-query-execution branch from 8625cfa to 96c85a3 Compare November 29, 2025 00:24
@alinaliBQ
Copy link
Contributor Author

alinaliBQ commented Nov 29, 2025

PR is rebased and ready for review

@alinaliBQ alinaliBQ marked this pull request as ready for review November 29, 2025 00:25
@alinaliBQ alinaliBQ requested a review from lidavidm as a code owner November 29, 2025 00:25
@lidavidm
Copy link
Member

lidavidm commented Nov 30, 2025

It appears it needs to be rebased again

@alinaliBQ alinaliBQ force-pushed the gh-47711-query-execution branch from 96c85a3 to 74544d8 Compare December 1, 2025 19:27
@alinaliBQ
Copy link
Contributor Author

alinaliBQ commented Dec 1, 2025

@lidavidm Yup, that might be a trend for these ODBC API PRs 😂 . I have rebased the PR and tested locally on MSVC

@alinaliBQ alinaliBQ force-pushed the gh-47711-query-execution branch from 74544d8 to 22d48eb Compare December 3, 2025 19:17
@lidavidm lidavidm merged commit b2e8f25 into apache:main Dec 4, 2025
45 checks passed
@lidavidm lidavidm removed the awaiting committer review Awaiting committer review label Dec 4, 2025
@lidavidm lidavidm mentioned this pull request Dec 4, 2025
Closed
@github-actions github-actions bot added the awaiting merge Awaiting merge label Dec 4, 2025
@alinaliBQ alinaliBQ mentioned this pull request Nov 10, 2025
Merged
@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Dec 18, 2025

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes

+1 more reviewer

@justing-bq justing-bq justing-bq left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting merge Awaiting merge Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alinaliBQ @lidavidm @justing-bq