Skip to content

GH-51114: [FlightSQL][C++] Expose is_update field of PreparedStatement - #51133

Open
SHIVANSH-ux-ys wants to merge 3 commits into
apache:mainfrom
SHIVANSH-ux-ys:feature-51114-flightsql-is-update
Open

GH-51114: [FlightSQL][C++] Expose is_update field of PreparedStatement#51133
SHIVANSH-ux-ys wants to merge 3 commits into
apache:mainfrom
SHIVANSH-ux-ys:feature-51114-flightsql-is-update

Conversation

@SHIVANSH-ux-ys

@SHIVANSH-ux-ys SHIVANSH-ux-ys commented Sep 2, 2026

Copy link
Copy Markdown

Fixes #51114.

Exposes the is_update boolean field in the C++ Flight SQL layer:

  • Added is_update field to ActionCreatePreparedStatementResult in server.h and updated PackActionResult in server.cc.
  • Added bool is_update = false parameter and is_update() const accessor to PreparedStatement in client.h/client.cc.
  • Updated PreparedStatement::ParseResponse to extract is_update.
  • Updated SQLite example server (sqlite_server.cc) to populate is_update via sqlite3_stmt_readonly.
  • Added assertions in server_test.cc for both query (ASSERT_FALSE) and update (ASSERT_TRUE) prepared statements.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51114 has no components, please add labels for components.

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems reasonable, but please fix the PR title/description.

Also CC @ennuite. Was there an integration test case we can enable? (I don't see one.)

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 2, 2026
@SHIVANSH-ux-ys SHIVANSH-ux-ys changed the title GH-51114: [FlightSQL][C++] Expose is_update field of PreparedStatemen… GH-51114: [FlightSQL][C++] Expose is_update field of PreparedStatement Sep 2, 2026
@SHIVANSH-ux-ys

Copy link
Copy Markdown
Author

Thanks @lidavidm! Updated the PR title and description.

Also added support to populate is_update in the SQLite example server (sqlite_server.cc) using sqlite3_stmt_readonly and added integration test assertions in server_test.cc for both TestCommandPreparedStatementQuery (ASSERT_FALSE) and TestCommandPreparedStatementUpdate (ASSERT_TRUE). CC @ennuite

@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51114 has no components, please add labels for components.

@ennuite ennuite 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.

I will do a deeper review tomorrow as I'm still familiarizing myself with the C++ codebase.

However, I don't see the same semantics as other PRs here. We need more than is_update being true/false, we need a presence check to know if the field was ever sent. This is important for backward compatibility (I assume most servers will not send this field).

The updates done to the tests also seem to not leave any test that doesn't set the field. IMO this is the most important test (again for backward compatibility).

@SHIVANSH-ux-ys I would like to preserve the true/false/unset semantics if possible.

@ennuite

ennuite commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for the cc @lidavidm

@SHIVANSH-ux-ys

Copy link
Copy Markdown
Author

Updated with the Doxygen parameter docstring for is_update in client.h. Ready for review / workflow approval when convenient @lidavidm @ennuite!

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.

[FlightSQL][C++] Expose is_update field of PreparedStatement

3 participants