Skip to content

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

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

Open
vikrantpuppala wants to merge 2 commits into
base: main
Choose a base branch
from
+89 −8
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6db4eb6
GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor …
vikrantpuppala Sep 15, 2026
af0af1e
GH-51341: Strengthen descriptor detachment coverage
vikrantpuppala Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ ODBCStatement::ODBCStatement(ODBCConnection& connection,
ird_(std::make_shared<ODBCDescriptor>(spi_statement_->GetDiagnostics(), nullptr,
this, false, false,
connection.IsOdbc2Connection())),
current_ard_(built_in_apd_.get()),
current_ard_(built_in_ard_.get()),
current_apd_(built_in_apd_.get()),
row_number_(0),
max_rows_(0),
Expand Down Expand Up @@ -323,6 +323,10 @@ void ODBCStatement::ExecuteDirect(const std::string& query) {

bool ODBCStatement::Fetch(size_t rows, SQLULEN* row_count_ptr,
SQLUSMALLINT* row_status_array) {
if (!current_result_) {
throw DriverException("Invalid cursor state", "24000");
}

if (has_reached_end_of_result_) {
ird_->SetRowsProcessed(0);
return false;
Expand Down Expand Up @@ -558,8 +562,9 @@ void ODBCStatement::SetStmtAttr(SQLINTEGER statement_attribute, SQLPOINTER value

switch (statement_attribute) {
case SQL_ATTR_APP_PARAM_DESC: {
ODBCDescriptor* desc = static_cast<ODBCDescriptor*>(value);
if (desc && current_apd_ != desc) {
ODBCDescriptor* desc =
value ? static_cast<ODBCDescriptor*>(value) : built_in_apd_.get();
if (current_apd_ != desc) {
if (current_apd_ != built_in_apd_.get()) {
current_apd_->DetachFromStatement(this, true);
}
Expand All @@ -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) {
if (current_ard_ != built_in_ard_.get()) {
current_ard_->DetachFromStatement(this, false);
}
Expand Down Expand Up @@ -740,6 +746,10 @@ void ODBCStatement::CloseCursor(bool suppress_errors) {
SQLRETURN ODBCStatement::GetData(SQLSMALLINT record_number, SQLSMALLINT c_type,
SQLPOINTER data_ptr, SQLLEN buffer_length,
SQLLEN* indicator_ptr) {
if (!current_result_) {
throw DriverException("Invalid cursor state", "24000");
}

if (record_number == 0) {
throw DriverException("Bookmarks are not supported", "07009");
} else if (static_cast<size_t>(record_number) > ird_->GetRecords().size()) {
Expand Down Expand Up @@ -785,6 +795,7 @@ SQLRETURN ODBCStatement::GetData(SQLSMALLINT record_number, SQLSMALLINT c_type,

SQLRETURN ODBCStatement::GetMoreResults() {
// Multiple result sets are not supported by Arrow protocol.
CloseCursor(/*suppress_errors=*/true);
return SQL_NO_DATA;
}

Expand All @@ -811,6 +822,12 @@ void ODBCStatement::GetRowCount(SQLLEN* row_count_ptr) {

void ODBCStatement::ReleaseStatement() {
CloseCursor(true);
if (current_apd_ != built_in_apd_.get()) {
current_apd_->DetachFromStatement(this, true);
}
if (current_ard_ != built_in_ard_.get()) {
current_ard_->DetachFromStatement(this, false);
}
connection_.DropStatement(this);
}

Expand Down
53 changes: 50 additions & 3 deletions cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ TYPED_TEST(ConnectionTest, TestSQLSetStmtAttrDescriptor) {

EXPECT_EQ(SQL_SUCCESS, SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, &internal_ard,
sizeof(internal_ard), 0));
EXPECT_NE(internal_apd, internal_ard);

// Set APD descriptor to explicitly allocated handle
EXPECT_EQ(SQL_SUCCESS, SQLSetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC,
Expand All @@ -549,12 +550,41 @@ TYPED_TEST(ConnectionTest, TestSQLSetStmtAttrDescriptor) {

EXPECT_EQ(ard_descriptor, value);

// Free explicitly allocated APD and ARD descriptor handles
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, apd_descriptor));
// 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));
Comment on lines +553 to +557
EXPECT_EQ(SQL_SUCCESS, SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC, &value,
sizeof(value), 0));
EXPECT_EQ(internal_apd, value);
EXPECT_EQ(SQL_SUCCESS,
SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, &value, sizeof(value), 0));
EXPECT_EQ(internal_ard, value);

// Assign replacement descriptors after the reset. Freeing the old descriptors must
// not revert these replacements, which proves the reset detached the old handles.
SQLHDESC replacement_apd, replacement_ard;
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DESC, this->conn, &replacement_apd));
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DESC, this->conn, &replacement_ard));
ASSERT_EQ(SQL_SUCCESS,
SQLSetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC, replacement_apd, 0));
ASSERT_EQ(SQL_SUCCESS,
SQLSetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, replacement_ard, 0));

ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, apd_descriptor));
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, ard_descriptor));
EXPECT_EQ(SQL_SUCCESS, SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC, &value,
sizeof(value), 0));
EXPECT_EQ(replacement_apd, value);
EXPECT_EQ(SQL_SUCCESS,
SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, &value, sizeof(value), 0));
EXPECT_EQ(replacement_ard, value);

ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, replacement_apd));
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, replacement_ard));

// Verify APD and ARD descriptors has been reverted to implicit descriptors
// Freeing the active explicit descriptors restores the implicit descriptors.
value = nullptr;

EXPECT_EQ(SQL_SUCCESS, SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_PARAM_DESC, &value,
Expand All @@ -568,4 +598,21 @@ TYPED_TEST(ConnectionTest, TestSQLSetStmtAttrDescriptor) {
EXPECT_EQ(internal_ard, value);
}

TYPED_TEST(ConnectionTest, TestExplicitDescriptorsOutliveStatement) {
SQLHSTMT statement;
SQLHDESC apd_descriptor, ard_descriptor;
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, this->conn, &statement));
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DESC, this->conn, &apd_descriptor));
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DESC, this->conn, &ard_descriptor));

ASSERT_EQ(SQL_SUCCESS,
SQLSetStmtAttr(statement, SQL_ATTR_APP_PARAM_DESC, apd_descriptor, 0));
ASSERT_EQ(SQL_SUCCESS,
SQLSetStmtAttr(statement, SQL_ATTR_APP_ROW_DESC, ard_descriptor, 0));

ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, statement));
EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, apd_descriptor));
EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, ard_descriptor));
}

} // namespace arrow::flight::sql::odbc
17 changes: 17 additions & 0 deletions cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1981,12 +1981,29 @@ TYPED_TEST(StatementTest, TestSQLMoreResultsNoData) {
ASSERT_EQ(SQL_SUCCESS, SQLExecDirect(this->stmt, wsql, wsql_len));

ASSERT_EQ(SQL_NO_DATA, SQLMoreResults(this->stmt));

// SQLMoreResults closes the current cursor when there is no next result.
ASSERT_EQ(SQL_ERROR, SQLCloseCursor(this->stmt));
VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, kErrorState24000);
}

TYPED_TEST(StatementTest, TestSQLMoreResultsWithoutQuery) {
ASSERT_EQ(SQL_NO_DATA, SQLMoreResults(this->stmt));
}

TYPED_TEST(StatementTest, TestSQLFetchWithoutCursor) {
ASSERT_EQ(SQL_ERROR, SQLFetch(this->stmt));
VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, kErrorState24000);
}

TYPED_TEST(StatementTest, TestSQLGetDataWithoutCursor) {
SQLINTEGER value;
SQLLEN indicator;
ASSERT_EQ(SQL_ERROR,
SQLGetData(this->stmt, 1, SQL_C_LONG, &value, sizeof(value), &indicator));
VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, kErrorState24000);
}

TYPED_TEST(StatementTest, TestSQLNativeSqlReturnsInputString) {
SQLWCHAR buf[1024];
SQLINTEGER buf_char_len = sizeof(buf) / GetSqlWCharSize();
Expand Down
Loading