feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult#732
feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult#732ennuite wants to merge 5 commits into
Conversation
|
NB: I'm still investigating what led to such a large diff in the protobuf generated file. The changes correspond only to compiling the PR at apache/arrow#49498 with the following command: @zeroshade am I supposed to merge changes to that file? I left them here for now to make the PR self contained for easier review, but I assume that if the PR in the main repo is merged, the generated code in this repo will be automatically updated. I will remove the changes if/when that time comes. |
The diff is likely due to the fact that we haven't regenerated the protobuf code for quite a while, and protoc is now several versions ahead of when we last regenerated the code. We also have a go generate that'll regenerate for you btw. I don't consider the diff in the protobuf to necessarily be bad though |
b8d7095 to
a432344
Compare
|
I rebased this onto main which should solve the CI issues, I'll also re-review it |
There was a problem hiding this comment.
Pull request overview
This PR extends the FlightSQL prepared statement API to surface a server-provided optional is_update hint (as *bool) so clients can choose the correct execution path (query vs update) when using prepared statements.
Changes:
- Added
IsUpdate *boolto the server-sideActionCreatePreparedStatementResultand mapped it into the protobuf action result. - Plumbed
IsUpdatethrough the client prepared statement parsing/loading path and exposed it viaPreparedStatement.IsUpdate(). - Added unit tests (client) and integration tests (server) covering both
is_update=false(query) andis_update=true(update) prepared statements.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
arrow/flight/flightsql/server.go |
Adds IsUpdate to the server result struct and forwards it for CreatePreparedStatement responses (but currently not for Substrait plan responses). |
arrow/flight/flightsql/client.go |
Stores/parses IsUpdate into PreparedStatement and exposes a getter; updates related documentation. |
arrow/flight/flightsql/client_test.go |
Adds unit tests verifying the client reads IsUpdate and executes the appropriate code path. |
arrow/flight/flightsql/server_test.go |
Adds integration tests (and server stubs) to validate end-to-end behavior for prepared statement query/update execution and the IsUpdate hint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query := req.GetQuery() | ||
| var isUpdate *bool | ||
| result.Handle = []byte(query) | ||
| switch query { | ||
| case "prepared query": | ||
| isUpdate := false | ||
| result.IsUpdate = &isUpdate | ||
| case "prepared update": | ||
| isUpdate := true | ||
| result.IsUpdate = &isUpdate | ||
| default: | ||
| err = fmt.Errorf("unknown query: %s", query) | ||
| } | ||
| if isUpdate != nil { | ||
| result.IsUpdate = isUpdate | ||
| } | ||
| return |
| if output.ParameterSchema != nil { | ||
| result.ParameterSchema = flight.SerializeSchema(output.ParameterSchema, f.mem) | ||
| } | ||
| // is_update is not relevant for prepared substrait plans | ||
|
|
| // If the server returned the Dataset Schema or Parameter Binding schemas | ||
| // at creation, they will also be accessible from this object. Close | ||
| // or isUpdate at creation, they will also be accessible from this object. Close | ||
| // should be called when no longer needed. |
Rationale for this change
Following the discussion in the mailing list
, this PR introduces a new field in
ActionCreatePreparedStatementResult,optional boolean is_update, that allows servers to indicate to clients the correct execution path for prepared statements.What changes are included in this PR?
The new field
is_updateis added toPreparedStatement, as well as serialization/deserialization and getters.Are these changes tested?
Yes. Unit tests for the new field were added to
client_test.go, that complement the already existing unit tests for prepared statements queries (there were no tests for prepared statement updates). Integration tests for the new field were added toserver_test.go. These are also the first integration tests for prepared statements.Are there any user-facing changes?
Yes. There are no breaking changes because the new field is optional, but a user-facing extension to prepared statements was added to allow for reading/writing to the new field.
AI Disclaimer
This change was created with AI assistance (Augment Code). All lines were manually reviewed by a human. The output is not copyrightable subject matter.
Closes #705