zeroshade commented on code in PR #732:
URL: https://github.com/apache/arrow-go/pull/732#discussion_r3659287031
##########
arrow/flight/flightsql/client.go:
##########
@@ -566,6 +566,7 @@ func (c *Client) LoadPreparedStatementFromResult(result
*CreatePreparedStatement
handle: result.PreparedStatementHandle,
datasetSchema: dsSchema,
paramSchema: paramSchema,
+ isUpdate: result.IsUpdate,
Review Comment:
Two small things on this propagation site.
1. **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.)
2. **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.
##########
arrow/flight/flightsql/client.go:
##########
@@ -1365,6 +1368,17 @@ func (p *PreparedStatement) DatasetSchema()
*arrow.Schema { return p.datasetSche
// the prepared statement.
func (p *PreparedStatement) ParameterSchema() *arrow.Schema { return
p.paramSchema }
+// IsUpdate reports the server's hint for how to execute this prepared
statement.
+// val is true if the server indicated an update, false if it indicated a
query.
+// ok is false if the server did not provide a hint, in which case the client
+// can choose how to execute the statement.
+func (p *PreparedStatement) IsUpdate() (val bool, ok bool) {
Review Comment:
**Endorsing this API shape explicitly**, since it has no precedent elsewhere
in `arrow/flight` and I'd rather it not get churned in a later review pass.
`(val, ok bool)` is the right call here:
- It's the direct analogue of the Java client's nullable `Boolean
isUpdate()` (guarded by `hasIsUpdate()`), so we're consistent cross-language.
- Returning `*bool` would hand out internal state and invite mutation; a
tri-state enum is overkill for one optional bool.
- It's plain idiomatic Go (`v, ok := m[k]`).
Also endorsing that this is **informational only** and that
`Execute`/`ExecuteUpdate` don't consult it. That matches the spec ("If not set,
the client can choose") and mirrors upstream layering: in Java it's the *JDBC
adapter* that consumes the hint to pick `StatementType`, not `FlightSqlClient`
itself. Our analogous consumer is arrow-adbc, outside this repo. Enforcing or
warning on mismatch here would be a behavior change nobody asked for.
The doc comment is clear about all three states. Nice.
##########
arrow/flight/flightsql/client_test.go:
##########
@@ -423,6 +423,123 @@ func (s *FlightSqlClientSuite)
TestPreparedStatementExecute() {
s.Equal(&emptyFlightInfo, info)
}
+func (s *FlightSqlClientSuite) TestPreparedStatementExecuteWithIsUpdateFalse()
{
Review Comment:
**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:
```go
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.
##########
arrow/flight/gen/flight/FlightSql.pb.go:
##########
@@ -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
Review Comment:
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:
1. 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.
2. 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.
##########
arrow/flight/flightsql/server.go:
##########
@@ -155,6 +155,9 @@ type ActionCreatePreparedStatementResult struct {
Handle []byte
DatasetSchema *arrow.Schema
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
Review Comment:
Not a defect, but this needs a **release note**, because the public API
impact is wider than this struct.
`types.go:882` has:
```go
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:
```go
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.
##########
arrow/flight/flightsql/server_test.go:
##########
@@ -133,6 +133,74 @@ func (*testServer) DoGetStatement(ctx context.Context,
ticket flightsql.Statemen
return
}
+func (*testServer) CreatePreparedStatement(ctx context.Context, req
flightsql.ActionCreatePreparedStatementRequest) (result
flightsql.ActionCreatePreparedStatementResult, err error) {
Review Comment:
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`):
```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.
##########
arrow/flight/flightsql/server_test.go:
##########
@@ -287,6 +355,69 @@ func (s *FlightSqlServerSuite) TestExecuteChunkError() {
}
}
+func (s *FlightSqlServerSuite) TestExecutePreparedStatementQuery() {
+ prep, err := s.cl.Prepare(context.TODO(), "prepared query")
+ s.Require().NoError(err)
+ defer prep.Close(context.TODO())
+
+ val, ok := prep.IsUpdate()
+ s.Require().True(ok)
+ s.False(val)
+
+ fi, err := prep.Execute(context.TODO())
+ s.Require().NoError(err)
+ ep := fi.GetEndpoint()
+ s.Require().Len(ep, 1)
+ fr, err := s.cl.DoGet(context.TODO(), ep[0].GetTicket())
+ s.Require().NoError(err)
+ var recs []arrow.RecordBatch
+ for fr.Next() {
+ rec := fr.RecordBatch()
+ rec.Retain()
+ defer rec.Release()
+ recs = append(recs, rec)
+ }
+ s.Require().NoError(fr.Err())
+ tbl := array.NewTableFromRecords(fr.Schema(), recs)
+ defer tbl.Release()
+ s.Assert().Equal(int64(2), tbl.NumRows())
+ s.Assert().Equal(int64(1), tbl.NumCols())
+ col := tbl.Column(0)
+ s.Assert().Equal("t1", col.Name())
+ s.Assert().Equal(2, col.Len())
+ s.Assert().Equal(1, col.NullN())
+ s.Assert().Equal(arrow.INT16, col.DataType().ID())
+ var n int
+ for _, arr := range col.Data().Chunks() {
+ data := array.NewInt16Data(arr.Data())
+ defer data.Release()
+ for i := 0; i < data.Len(); i++ {
+ switch n {
+ case 0:
+ s.True(data.IsNull(i))
+ case 1:
+ s.False(data.IsNull(i))
+ s.Assert().Equal(int16(1), data.Value(i))
+ }
+ n++
+ }
+ }
+}
+
+func (s *FlightSqlServerSuite) TestExecutePreparedStatementUpdate() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
