You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vikrantpuppala (via GitHub)" <gi...@apache.org> on 2026/09/15 08:48:31 UTC
[PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
vikrantpuppala opened a new pull request, #51342:
URL: https://github.com/apache/arrow/pull/51342
### Rationale for this change
The Flight SQL ODBC statement implementation had a cluster of related descriptor and cursor lifecycle defects:
- the implicit application row descriptor (ARD) was initialized from the application parameter descriptor (APD), aliasing two descriptor roles;
- `SQL_NULL_HANDLE` did not restore built-in application descriptors;
- freeing a statement left dangling registrations in explicit descriptors;
- `SQLFetch` and `SQLGetData` without an open cursor could dereference a null result or report the wrong diagnostic; and
- `SQLMoreResults` did not close the current cursor when returning `SQL_NO_DATA`.
### What changes are included in this PR?
- Initialize the active ARD from `built_in_ard_`.
- Map null APD/ARD assignments to the corresponding built-in descriptor and detach the outgoing explicit descriptor.
- Detach explicit descriptors before releasing a statement.
- Return SQLSTATE `24000` from `SQLFetch` and `SQLGetData` when no cursor is open.
- Close the current cursor when `SQLMoreResults` returns `SQL_NO_DATA`.
- Add public ODBC API regressions for distinct implicit descriptors, null-handle restoration, statement-before-descriptor release order, invalid cursor calls, and `SQLMoreResults` cursor closure.
### Are there any user-facing changes?
Yes. Spec-valid descriptor release and reset sequences no longer retain dangling state, invalid cursor operations return SQLSTATE `24000`, and `SQLMoreResults` closes the current cursor as required.
### How was this patch tested?
- `clang-format` 19.1.7
- `git diff --check`
- Added focused mock/remote typed ODBC regression tests.
A fresh local CMake test configuration was blocked before compilation by the host dependency setup (a Boost CMake alias collision followed by unavailable gRPC); the full ODBC suite is left to Arrow CI.
Closes #51341.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51342:
URL: https://github.com/apache/arrow/pull/51342#discussion_r4014778819
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -549,6 +550,18 @@ TYPED_TEST(ConnectionTest, TestSQLSetStmtAttrDescriptor) {
EXPECT_EQ(ard_descriptor, value);
+ // A null descriptor handle restores the corresponding implicit descriptor.
+ EXPECT_EQ(SQL_SUCCESS,
+ SQLSetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC, SQL_NULL_HANDLE, 0));
+ EXPECT_EQ(SQL_SUCCESS,
+ SQLSetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, SQL_NULL_HANDLE, 0));
Review Comment:
The new assertions verify that a null handle selects the implicit descriptor, but they do not verify that the outgoing explicit descriptor was detached. If `DetachFromStatement` were omitted, this test would still pass; freeing that old descriptor later could call `RevertAppDescriptor` and overwrite a newer descriptor assignment. Please assign a second explicit APD/ARD after the null reset, free the old descriptors, and assert that `SQLGetStmtAttr` still returns the second handles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "vikrantpuppala (via GitHub)" <gi...@apache.org>.
vikrantpuppala commented on PR #51342:
URL: https://github.com/apache/arrow/pull/51342#issuecomment-6161531113
@lidavidm gentle ping here as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #51342:
URL: https://github.com/apache/arrow/pull/51342
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51342:
URL: https://github.com/apache/arrow/pull/51342#issuecomment-5677391864
:warning: GitHub issue #51341 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "vikrantpuppala (via GitHub)" <gi...@apache.org>.
vikrantpuppala commented on code in PR #51342:
URL: https://github.com/apache/arrow/pull/51342#discussion_r4279721155
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc:
##########
@@ -571,8 +576,9 @@ void ODBCStatement::SetStmtAttr(SQLINTEGER statement_attribute, SQLPOINTER value
return;
}
case SQL_ATTR_APP_ROW_DESC: {
- ODBCDescriptor* desc = static_cast<ODBCDescriptor*>(value);
- if (desc && current_ard_ != desc) {
+ ODBCDescriptor* desc =
+ value ? static_cast<ODBCDescriptor*>(value) : built_in_ard_.get();
+ if (current_ard_ != desc) {
Review Comment:
Fixed in 550eea61ab. Switching the active ARD now marks the incoming descriptor bindings as changed, and restoring the implicit ARD does the same. I added fetch/switch/reset/fetch and active-descriptor-free regressions through the direct driver API, plus manager-level coverage. The full local ODBC suite passes (394 passed, 322 skipped).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #51342:
URL: https://github.com/apache/arrow/pull/51342#issuecomment-6182802846
Can we rebase here too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "vikrantpuppala (via GitHub)" <gi...@apache.org>.
vikrantpuppala commented on PR #51342:
URL: https://github.com/apache/arrow/pull/51342#issuecomment-5809151177
@lidavidm could we trigger the ODBC CI here and see if we can get this merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51342:
URL: https://github.com/apache/arrow/pull/51342#discussion_r4278344748
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc:
##########
@@ -571,8 +576,9 @@ void ODBCStatement::SetStmtAttr(SQLINTEGER statement_attribute, SQLPOINTER value
return;
}
case SQL_ATTR_APP_ROW_DESC: {
- ODBCDescriptor* desc = static_cast<ODBCDescriptor*>(value);
- if (desc && current_ard_ != desc) {
+ ODBCDescriptor* desc =
+ value ? static_cast<ODBCDescriptor*>(value) : built_in_ard_.get();
+ if (current_ard_ != desc) {
Review Comment:
After fetching with the implicit ARD, switching to an explicit ARD and fetching, then restoring `SQL_NULL_HANDLE`, the implicit ARD's `HaveBindingsChanged()` flag is still false. The next fetch skips rebinding and continues writing into the detached explicit descriptor's buffers, potentially accessing released storage. Mark the incoming ARD's bindings as changed whenever the active descriptor changes, and add a fetch/reset/fetch regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle [arrow]
Posted by "vikrantpuppala (via GitHub)" <gi...@apache.org>.
vikrantpuppala commented on PR #51342:
URL: https://github.com/apache/arrow/pull/51342#issuecomment-6185833770
@lidavidm Rebased onto current main and addressed the latest review findings in 550eea61ab. This marks ARD bindings dirty when switching or restoring descriptors, adds direct-driver and manager-level rebinding regressions, and corrects driver-manager no-cursor expectations while retaining direct-driver `24000` coverage. The full local ODBC suite passes: 394 passed, 322 skipped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org