feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult by ennuite · Pull Request #732 · apache/arrow-go

ennuite · 2026-03-27T16:57:35Z

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_update is added to PreparedStatement, 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 to server_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

ennuite · 2026-03-27T17:04:55Z

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:

cd /path/to/arrow-go/fork/arrow-go/arrow/flight

protoc --experimental_allow_proto3_optional \
  -I/path/to/arrow/fork/arrow/format \
  --go_out=./gen/flight \
  --go-grpc_out=./gen/flight \
  --go_opt=paths=source_relative \
  --go-grpc_opt=paths=source_relative \
  /path/to/arrow/fork/arrow/format/FlightSql.proto

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

zeroshade · 2026-03-27T20:16:44Z

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:

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

zeroshade · 2026-06-08T16:23:52Z

I rebased this onto main which should solve the CI issues, I'll also re-review it

Copilot

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 *bool to the server-side ActionCreatePreparedStatementResult and mapped it into the protobuf action result.
Plumbed IsUpdate through the client prepared statement parsing/loading path and exposed it via PreparedStatement.IsUpdate().
Added unit tests (client) and integration tests (server) covering both is_update=false (query) and is_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.

ennuite · 2026-07-03T02:35:18Z

+	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


Good catch. This was also highlighted by Matt and I addressed it in #732 (comment)

 		if output.ParameterSchema != nil {
 			result.ParameterSchema = flight.SerializeSchema(output.ParameterSchema, f.mem)
 		}
+		// is_update is not relevant for prepared substrait plans



ennuite · 2026-07-13T01:36:27Z

 // 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.


Addressed in fcecddd

zeroshade

Thanks for the PR and the thorough tests — the feature looks good.

(Edit: withdrawing my earlier concern about the regenerated FlightSql.pb.go toolchain — that's fine as-is, no need to regenerate.)

One thing to tidy before merge — see the inline comment on server_test.go: the isUpdate := … declarations inside the cases shadow the outer var isUpdate *bool, so the trailing if isUpdate != nil block is dead code. Dropping the outer var and that block keeps the helper correct and clear.

I also left an optional, non-blocking note on the IsUpdate() *bool getter shape — take it or leave it.

Happy to approve once the test helper is cleaned up.

zeroshade · 2026-06-11T16:57:33Z

+	default:
+		err = fmt.Errorf("unknown query: %s", query)
+	}
+	if isUpdate != nil {


The isUpdate := … declarations inside the two cases above shadow the outer var isUpdate *bool, so this if isUpdate != nil always sees nil — it's dead code. The cases already set result.IsUpdate directly, so the outer var isUpdate *bool and this block can both be removed.

Thank you for this review.
Addressed in fc85381

zeroshade · 2026-06-11T16:57:33Z

+// IsUpdate returns the server's hint about whether this prepared statement
+// should be executed as an update (true) or query (false). If nil, the server
+// did not provide a hint and the client can choose how to execute the statement.
+func (p *PreparedStatement) IsUpdate() *bool { return p.isUpdate }


Returning the internal *bool leaks mutable state (a caller can do *ps.IsUpdate() = false and mutate the statement) and isn't idiomatic for an optional. Prefer IsUpdate() (val bool, ok bool), or return a copy.

Thank you for pointing this out. I took the idiomatic approach with the double boolean return, see 585c00e

zeroshade · 2026-06-19T20:13:13Z

@ennuite are you still working on this?

ennuite · 2026-06-30T16:36:20Z

@zeroshade Sorry, I will resume work on this now and will address the comments this week.

ennuite · 2026-07-03T03:05:44Z

@zeroshade I addressed your review comments, and refactored some other poor patterns in the tests.

I will put this as ready for review again once I update apache/arrow-adbc#4161 with the changes I did now and ran end-to-end tests to ensure that everything is ok.

…paredStatements

zeroshade

Thanks for pushing this through — and thanks especially for the thorough test additions; the prepared-statement integration tests are a genuine gap this fills independently of the feature itself.

I verified the change fairly aggressively and the feature itself is correct. Summary of what I checked:

Check	Result
Spec conformance vs upstream `apache/arrow` `format/FlightSql.proto`	`optional bool is_update = 4`; doc comments match the spec verbatim
Upstream status	apache/arrow#49498, merged 2026-06-09 — canonical, not a proposal
Generated-file semantic delta (all wire tags / exported methods / types / enum constants, vs true merge-base)	Exactly +1 wire tag, +1 getter, 0 type changes, 0 enum changes
Runtime descriptor	`#4 is_update kind=bool hasPresence=true`
Presence round-trip	`true`, `false`, and unset all round-trip correctly — importantly `false` survives the wire, confirming real presence semantics
`go build` / `go vet` / `go test ./arrow/flight/...`	All clean; the 4 new tests confirmed executing

So I want to be clear that the -868 net lines in FlightSql.pb.go is not hiding anything — I checked that specifically.

I'm marking this request changes for one substantive reason only: the unset/nil case is never asserted anywhere. Every new test asserts ok == true, but ok == false is the backward-compatibility contract with the (currently overwhelming majority of) servers that don't send the hint. That's the one path most likely to regress silently and it's the cheapest possible test to add.

Everything else below is either a release-note item or a nit. Two design choices I want to explicitly endorse so they don't get second-guessed in later review rounds: the (val, ok bool) getter shape, and keeping the hint purely informational. Details inline.

zeroshade · 2026-07-27T17:02:31Z

 	s.Equal(&emptyFlightInfo, info)
 }

+func (s *FlightSqlClientSuite) TestPreparedStatementExecuteWithIsUpdateFalse() {


Please add a test for the unset/nil case. This is my only blocking request.

Both new client tests here, and both new server tests, assert ok == true. Nothing anywhere asserts ok == false. That's the path taken by every server that doesn't implement the new field — i.e. the backward-compatibility contract — and it's currently the only untested branch of the getter.

TestPreparedStatementExecute above (line 368) already builds an ActionCreatePreparedStatementResult with no IsUpdate, so this can be as small as adding two lines there:

val, ok := prepared.IsUpdate() s.False(ok) s.False(val)

Worth locking down explicitly because a future refactor that switched isUpdate *bool to a plain bool would silently turn "no hint" into "is a query" — which is a real behavior change for clients that branch on this — and no existing test would catch it.

Do we want the s.False(val) check? If s.False(ok), then val is meaningless.

I addressed this in a324e27

I do not think it makes sense to have the check in val: it being False seems like an implementation detail, and not a part of the contract of the IsUpdate() method. Let me know if you disagree: I can change it to look exactly like your suggestion.

zeroshade · 2026-07-27T17:02:31Z

 		handle:        result.PreparedStatementHandle,
 		datasetSchema: dsSchema,
 		paramSchema:   paramSchema,
+		isUpdate:      result.IsUpdate,


Two small things on this propagation site.

Untested. TestPreparedStatementLoadFromResult (client_test.go:971) is the only coverage for LoadPreparedStatementFromResult, and it doesn't assert IsUpdate. Since that test already constructs the result and checks the other two propagated fields (ParameterSchema, DatasetSchema), adding an IsUpdate assertion there would be consistent and nearly free. (Couldn't leave this as an inline comment on that line — it's outside the diff.)

Nit, no change required: this is the one place that stores a *bool owned by the caller, so a caller mutating result.IsUpdate after this call would change the statement's view. Prepare/PrepareSubstrait don't have this property since they point at their own unmarshaled local. Not worth a defensive copy unless you'd want it for handle too — just noting it so the asymmetry is a conscious choice.

I addressed this with 2 tests. In TestPreparedStatementLoadFromResult I let things as they are, and tested with the new field unset, see 514d3c6. Note that this test has the same nuance about val that I pointed out in feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult #732 (comment)

I added a new test that checks that the field is properly loaded also when set, see a334b86

For that one I purposefully followed the example set by handle, but I will admit I had my own reservations while doing it.

I will not change this in this PR unless you ask me to, but in your opinion is it worth a follow-up PR to have a defensive copy for both handle and isUpdate? If so I can tackle that after this one

zeroshade · 2026-07-27T17:02:31Z

 	ParameterSchema *arrow.Schema
+	// IsUpdate indicates whether the prepared statement should be executed
+	// as an update (true) or query (false). If nil, the client can choose.
+	IsUpdate *bool


Not a defect, but this needs a release note, because the public API impact is wider than this struct.

types.go:882 has:

type CreatePreparedStatementResult = pb.ActionCreatePreparedStatementResult

Being a type alias, the regenerated proto field lands on a public flightsql symbol too. So this PR adds an exported field to two publicly-reachable structs. Keyed composite literals are unaffected (and that's what I found in the downstream usage I spot-checked), but any downstream unkeyed literal like:

return flightsql.ActionCreatePreparedStatementResult{handle, ds, ps}, nil

will stop compiling. That's permitted under the Go 1 compatibility rules — unkeyed literals of imported struct types are explicitly not covered — and go vet's composites check flags them, so real-world exposure should be low. I don't think it should block the PR, but it's worth a line in the changelog rather than letting someone discover it at upgrade time.

The unconditional result.IsUpdate = output.IsUpdate on both the statement and Substrait branches is correct, by the way — both sides are *bool, so nil flows through and no presence check is needed. Good that the Substrait path wasn't forgotten.

zeroshade · 2026-07-27T17:02:31Z

@@ -17,19 +17,19 @@

 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-// 	protoc-gen-go v1.31.0
-// 	protoc        v4.25.3
+// 	protoc-gen-go v1.36.11


This regeneration jumped the toolchain: protoc-gen-go v1.31.0 -> v1.36.11 and protoc v4.25.3 -> v7.34.0, while the sibling Flight.pb.go and Flight_grpc.pb.go stay on the old versions. That's what produces the +765/-1633 diff on a one-field change.

I am not asking you to revert it, and I want to record that I verified it's safe, so this doesn't get re-litigated:

I diffed every protobuf:"..." wire tag, every exported generated method, every generated type, and every enum constant against the true merge-base. The only deltas are the is_update tag and GetIsUpdate(). Zero other semantic change.

Both the old and new files carry the same protoimpl.EnforceVersion(20 ...) guards, so this does not raise the protobuf runtime floor.

go.mod already requires google.golang.org/protobuf v1.36.11, so the new codegen actually matches our pinned runtime — Flight.pb.go is the stale one, not this file.

Two requests, both cheap:

Please note the exact protoc / protoc-gen-go versions you used in the PR description, and if it's not already the case, keep the regeneration as its own commit so the feature commit stays reviewable in isolation.

Please don't also regenerate Flight.pb.go here — that belongs in a separate cleanup PR.

The root cause isn't yours: arrow/flight/gen.go generates from -I../../../format (the proto isn't vendored) and we have no CI check that generated files are current, so version drift like this is invited. I'll open a follow-up issue for pinning the generator and adding a regeneration check — no action needed from you.

Filed the toolchain follow-up as #1029 — pinning the generators, documenting where format/*.proto comes from, and adding a CI check that regenerates and fails on diff. Nothing there is asked of this PR; the two requests in this thread still stand as-is (note the versions you used, keep the regen in its own commit).

Also captured there: please don't regenerate Flight.pb.go/Flight_grpc.pb.go here — normalizing all four files is tracked as its own mechanical PR in #1029.

zeroshade · 2026-07-27T17:02:31Z

 	return
 }

+func (*testServer) CreatePreparedStatement(ctx context.Context, req flightsql.ActionCreatePreparedStatementRequest) (result flightsql.ActionCreatePreparedStatementResult, err error) {


Since you're already switching on the query string here, adding a third case with no hint set would give end-to-end coverage of the ok == false path through the real server/client round-trip (which is stronger than the mock-level test I asked for in client_test.go):

case "prepared no hint": // leave result.IsUpdate nil - server provides no hint

Then a short test asserting IsUpdate() returns (false, false). Pairs naturally with the two tests you added below.

Thanks for wiring up CreatePreparedStatement/DoGetPreparedStatement/DoPutPreparedStatementUpdate on testServer — I confirmed this doesn't disturb the existing TestDoGet/DoGetPreparedStatement expectations in the unimplemented-server suite, and the whole ./arrow/flight/... suite still passes.

zeroshade · 2026-07-27T17:02:31Z

+	}
+}
+
+func (s *FlightSqlServerSuite) TestExecutePreparedStatementUpdate() {


Nit / optional: the Substrait path (CreatePreparedSubstraitPlan on the server, PrepareSubstrait on the client) also carries the hint now but has no coverage. Residual risk is low since it shares parsePreparedStatementResponse with the path you did test, so I wouldn't hold the PR on it — just flagging in case you'd rather close the loop while you're in here.

Separately: the s.Assert().Equal(true, x) -> s.True(x) cleanups in this file are unrelated to the feature. They're strict improvements so I'm not going to ask you to strip them, but ideally they'd be a separate commit.

ennuite · 2026-07-29T00:51:57Z

Thanks for the very thorough review!

I'm going over the comments one by one, I might be a bit slow as I'm ramping up on some of the Go concepts, and your feedback has been a great learning experience.

I appreciate your patience as I improve the PR. I will continue working on it in the next few days, and at the end I'll try to tackle #1029 as well.

…Result

…om result

ennuite changed the title ~~feat(arrow/flight): Add is_update field to ActionCreatePreparedStatementResult~~ feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult Mar 27, 2026

ennuite mentioned this pull request Mar 31, 2026

feat(go/adbc/driver/flightsql): handle is_update in prepared stmts apache/arrow-adbc#4161

Open

alamb mentioned this pull request Jun 5, 2026

[FlightSQL] Add is_update field to ActionCreatePreparedStatementResult apache/arrow#49497

Closed

ennuite marked this pull request as ready for review June 8, 2026 14:22

ennuite requested a review from zeroshade as a code owner June 8, 2026 14:22

zeroshade force-pushed the gh-705-add-is-update-field branch from b8d7095 to a432344 Compare June 8, 2026 16:23

zeroshade requested a review from Copilot June 8, 2026 16:23

Copilot started reviewing on behalf of zeroshade June 8, 2026 16:23 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

zeroshade requested changes Jun 11, 2026

View reviewed changes

ennuite marked this pull request as draft July 3, 2026 02:27

ennuite added 12 commits July 26, 2026 23:04

Add compiled files from new proto definition

b3ce6c4

Extend FlightSQL client and server to have new is_update field in Pre…

cf15f96

…paredStatements

Add client tests for prepared statements with the new field

8651cbd

Add test for prepared statement query

d3595fd

Add test for PreparedStatementUpdate

c378e8e

Remove dead code from test

c38caff

Fix state leak

140b575

Fail fast in client test when server does not set is_update

21ab0bb

Refactor test assertions with booleans

bd61a0e

Improve comments

24e2862

Remove unrelated comment change

7437f87

Allow Substrait to also use the new field

f6a8588

ennuite force-pushed the gh-705-add-is-update-field branch from a26b6b3 to f6a8588 Compare July 26, 2026 22:10

Fix comment wrap-around

9670cca

ennuite marked this pull request as ready for review July 27, 2026 01:08

zeroshade requested changes Jul 27, 2026

View reviewed changes

zeroshade mentioned this pull request Jul 27, 2026

[Dev Tools][CI] Pin protobuf codegen toolchain and verify generated .pb.go files are up to date #1029

Open

Add coverage of unset isUpdate field in client unit test

a324e27

ennuite added 2 commits July 29, 2026 04:10

Add coverage of unset isUpdate field in TestPreparedStatementLoadFrom…

514d3c6

…Result

Add coverage of set isUpdate field when loading prepared statement fr…

a334b86

…om result

Uh oh!

Conversation

ennuite commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

AI Disclaimer

Uh oh!

ennuite commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade commented Mar 27, 2026

Uh oh!

zeroshade commented Jun 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ennuite Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Jun 19, 2026

Uh oh!

ennuite commented Jun 30, 2026

Uh oh!

ennuite commented Jul 3, 2026

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ennuite Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ennuite commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ennuite commented Mar 27, 2026 •

edited

Loading

ennuite commented Mar 27, 2026 •

edited

Loading

zeroshade left a comment •

edited

Loading

ennuite Jul 3, 2026 •

edited

Loading

ennuite Jul 29, 2026 •

edited

Loading

ennuite commented Jul 29, 2026 •

edited

Loading