ennuite commented on code in PR #732:
URL: https://github.com/apache/arrow-go/pull/732#discussion_r3670708880
##########
arrow/flight/flightsql/client_test.go:
##########
@@ -423,6 +423,123 @@ func (s *FlightSqlClientSuite)
TestPreparedStatementExecute() {
s.Equal(&emptyFlightInfo, info)
}
+func (s *FlightSqlClientSuite) TestPreparedStatementExecuteWithIsUpdateFalse()
{
Review Comment:
I addressed this in
https://github.com/apache/arrow-go/pull/732/changes/eb160883859a2843fdfa2a76598393efac2eb9b1
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.
##########
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:
1. I addressed this with 2 tests. In `TestPreparedStatementLoadFromResult` I
let things as they are, and tested with the new field unset, see
https://github.com/apache/arrow-go/pull/732/changes/8ba207b4402f60ce27cbad99282eebb8c649e50e.
Note that this test has the same nuance about `val` that I pointed out in
https://github.com/apache/arrow-go/pull/732#discussion_r3670708880
I added a new test that checks that the field is properly loaded also when
set, see
https://github.com/apache/arrow-go/pull/732/changes/a334b86f6c7dbc7e1fc33acc014e5b70b5201e23
2. 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
##########
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:
Addressed this in
https://github.com/apache/arrow-go/pull/732/changes/786adc73394a5d1f1987f3c122a6def3d28d1823
I purposefully do not execute the query: since the hint is not set, it is up
to the client to decide whether to execute with `DoGet` or `DoPut`. I do not
want the test to misguide someone to think that one way is preferred.
The same nuance about checking `val` when `ok` is `False` is also present in
this test (see
https://github.com/apache/arrow-go/pull/732#discussion_r3670708880)
##########
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:
1. About the Substrait path: I added server-side tests at
4816b1b718db17f018f94f57d0f84ab1c64252ff
I don't actually execute the queries because, apart from Prepared Statement
creation, Substrait and SQL have the exact same execution path in Flight SQL,
the difference is only in how the query is sent. Executing the queries would
just be exercising the same code path unnecessarily.
I note that I didn't find any client-side tests for Substrait, and I think
if we need them then they should go in a separate issue/PR. Do you agree?
2. Yeah, the cleanups outside my own tests were a Find->Replace mistake: I
was trying to fix my own tests and did not realize I changed other ones. I'll
be more careful next time.
##########
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:
Addressed in fcde3da8de20361c11ece62b2ba914d49cfd906f
--
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]
