Copilot commented on code in PR #732:
URL: https://github.com/apache/arrow-go/pull/732#discussion_r3374798188
##########
arrow/flight/flightsql/client.go:
##########
@@ -1105,13 +1107,14 @@ func (tx *Txn) RollbackSavepoint(ctx context.Context,
sp Savepoint, opts ...grpc
// prepared statement handle.
//
// 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.
Review Comment:
The PreparedStatement type doc comment refers to "isUpdate" (lower
camel-case) and the sentence is grammatically unclear. Since the exported API
is `IsUpdate()` and the concept is a server hint, adjust the wording to clearly
describe the new field alongside the existing schema fields.
##########
arrow/flight/flightsql/server_test.go:
##########
@@ -133,6 +133,78 @@ func (*testServer) DoGetStatement(ctx context.Context,
ticket flightsql.Statemen
return
}
+func (*testServer) CreatePreparedStatement(ctx context.Context, req
flightsql.ActionCreatePreparedStatementRequest) (result
flightsql.ActionCreatePreparedStatementResult, err error) {
+ 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
Review Comment:
In CreatePreparedStatement, `isUpdate` is declared as `*bool` but then
shadowed inside the switch (`isUpdate := ...`). This makes the final `if
isUpdate != nil { ... }` block dead code and obscures the intent (it never
copies a value). Simplify by removing the outer variable and assigning directly
to `result.IsUpdate` without shadowing.
##########
arrow/flight/flightsql/server.go:
##########
@@ -1286,6 +1290,7 @@ func (f *flightSqlServer) DoAction(cmd *flight.Action,
stream flight.FlightServi
if output.ParameterSchema != nil {
result.ParameterSchema =
flight.SerializeSchema(output.ParameterSchema, f.mem)
}
+ // is_update is not relevant for prepared substrait plans
Review Comment:
The CreatePreparedSubstraitPlan action returns
`pb.ActionCreatePreparedStatementResult`, which now includes `is_update`. This
handler currently drops `output.IsUpdate` (and even states it is not relevant),
so servers cannot propagate the hint for prepared Substrait plans even if they
set it in `ActionCreatePreparedStatementResult`. Forward `output.IsUpdate` to
the protobuf result for consistency with CreatePreparedStatement.
--
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]
