Skip to content

GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle - #51342

Open
vikrantpuppala wants to merge 2 commits into
apache:mainfrom
vikrantpuppala:stmt-descriptor-cursor-lifecycle
Open

vikrantpuppala wants to merge 2 commits into
apache:mainfrom
vikrantpuppala:stmt-descriptor-cursor-lifecycle

Conversation

@vikrantpuppala

@vikrantpuppala vikrantpuppala commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

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.

@github-actions

Copy link
Copy Markdown

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The null-handle tests do not verify that outgoing explicit descriptors are actually detached.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes ODBC statement descriptor ownership and cursor lifecycle behavior.

Changes:

  • Corrects implicit ARD initialization and descriptor reset/detachment.
  • Adds invalid-cursor diagnostics and closes exhausted result cursors.
  • Adds descriptor and cursor lifecycle regression tests.
File summaries
File Description
odbc_statement.cc Implements lifecycle fixes.
connection_test.cc Tests descriptor handling.
statement_test.cc Tests cursor diagnostics and closure.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +553 to +557
// 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));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle

2 participants