-
Notifications
You must be signed in to change notification settings - Fork 136
feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 13 commits
b3ce6c4
cf15f96
8651cbd
d3595fd
c378e8e
c38caff
140b575
21ab0bb
bd61a0e
24e2862
7437f87
f6a8588
9670cca
a324e27
514d3c6
a334b86
221f7d6
271a209
3009533
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,6 +423,123 @@ func (s *FlightSqlClientSuite) TestPreparedStatementExecute() { | |
| s.Equal(&emptyFlightInfo, info) | ||
| } | ||
|
|
||
| func (s *FlightSqlClientSuite) TestPreparedStatementExecuteWithIsUpdateFalse() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test for the unset/ Both new client tests here, and both new server tests, assert
val, ok := prepared.IsUpdate()
s.False(ok)
s.False(val)Worth locking down explicitly because a future refactor that switched
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I addressed this in a324e27 I do not think it makes sense to have the check in |
||
| const query = "query" | ||
|
|
||
| cmd := &pb.ActionCreatePreparedStatementRequest{Query: query} | ||
| action := getAction(cmd) | ||
| action.Type = flightsql.CreatePreparedStatementActionType | ||
| closeAct := getAction(&pb.ActionClosePreparedStatementRequest{PreparedStatementHandle: []byte(query)}) | ||
| closeAct.Type = flightsql.ClosePreparedStatementActionType | ||
|
|
||
| isUpdate := false | ||
| result := &pb.ActionCreatePreparedStatementResult{ | ||
| PreparedStatementHandle: []byte(query), // handle is the query string for simplicity | ||
| IsUpdate: &isUpdate, | ||
| } | ||
| var out anypb.Any | ||
| out.MarshalFrom(result) | ||
| data, _ := proto.Marshal(&out) | ||
|
|
||
| createRsp := &mockDoActionClient{} | ||
| defer createRsp.AssertExpectations(s.T()) | ||
| createRsp.On("Recv").Return(&pb.Result{Body: data}, nil).Once() | ||
| createRsp.On("Recv").Return(&pb.Result{}, io.EOF).Once() | ||
| createRsp.On("CloseSend").Return(nil).Once() | ||
|
|
||
| closeRsp := &mockDoActionClient{} | ||
| defer closeRsp.AssertExpectations(s.T()) | ||
| closeRsp.On("Recv").Return(&pb.Result{}, io.EOF) | ||
| closeRsp.On("CloseSend").Return(nil) | ||
|
|
||
| s.mockClient.On("DoAction", flightsql.CreatePreparedStatementActionType, action.Body, s.callOpts). | ||
| Return(createRsp, nil).Once() | ||
| s.mockClient.On("DoAction", flightsql.ClosePreparedStatementActionType, closeAct.Body, s.callOpts). | ||
| Return(closeRsp, nil) | ||
|
|
||
| infoCmd := &pb.CommandPreparedStatementQuery{PreparedStatementHandle: []byte(query)} | ||
| desc := getDesc(infoCmd) | ||
| s.mockClient.On("GetFlightInfo", desc.Type, desc.Cmd, s.callOpts).Return(&emptyFlightInfo, nil).Once() | ||
|
|
||
| prepared, err := s.sqlClient.Prepare(context.TODO(), query, s.callOpts...) | ||
| s.NoError(err) | ||
| defer prepared.Close(context.TODO(), s.callOpts...) | ||
|
|
||
| s.Equal(string(prepared.Handle()), query) | ||
| val, ok := prepared.IsUpdate() | ||
| s.Require().True(ok) | ||
| s.False(val) | ||
|
|
||
| info, err := prepared.Execute(context.TODO(), s.callOpts...) | ||
| s.NoError(err) | ||
| s.Equal(&emptyFlightInfo, info) | ||
| } | ||
|
|
||
| func (s *FlightSqlClientSuite) TestPreparedStatementExecuteUpdateWithIsUpdateTrue() { | ||
| const query = "DML query" | ||
|
|
||
| cmd := &pb.ActionCreatePreparedStatementRequest{Query: query} | ||
| action := getAction(cmd) | ||
| action.Type = flightsql.CreatePreparedStatementActionType | ||
| closeAct := getAction(&pb.ActionClosePreparedStatementRequest{PreparedStatementHandle: []byte(query)}) | ||
| closeAct.Type = flightsql.ClosePreparedStatementActionType | ||
|
|
||
| // Set is_update to true | ||
| isUpdate := true | ||
| result := &pb.ActionCreatePreparedStatementResult{ | ||
| PreparedStatementHandle: []byte(query), | ||
| IsUpdate: &isUpdate, | ||
| } | ||
| var out anypb.Any | ||
| out.MarshalFrom(result) | ||
| data, _ := proto.Marshal(&out) | ||
|
|
||
| createRsp := &mockDoActionClient{} | ||
| defer createRsp.AssertExpectations(s.T()) | ||
| createRsp.On("Recv").Return(&pb.Result{Body: data}, nil).Once() | ||
| createRsp.On("Recv").Return(&pb.Result{}, io.EOF).Once() | ||
| createRsp.On("CloseSend").Return(nil).Once() | ||
|
|
||
| closeRsp := &mockDoActionClient{} | ||
| defer closeRsp.AssertExpectations(s.T()) | ||
| closeRsp.On("Recv").Return(&pb.Result{}, io.EOF) | ||
| closeRsp.On("CloseSend").Return(nil) | ||
|
|
||
| s.mockClient.On("DoAction", flightsql.CreatePreparedStatementActionType, action.Body, s.callOpts). | ||
| Return(createRsp, nil).Once() | ||
| s.mockClient.On("DoAction", flightsql.ClosePreparedStatementActionType, closeAct.Body, s.callOpts). | ||
| Return(closeRsp, nil) | ||
|
|
||
| // Mock DoPut for ExecuteUpdate | ||
| updateCmd := &pb.CommandPreparedStatementUpdate{PreparedStatementHandle: []byte(query)} | ||
| updateDesc := getDesc(updateCmd) | ||
| updateResult := &pb.DoPutUpdateResult{RecordCount: 1} | ||
| resdata, _ := proto.Marshal(updateResult) | ||
|
|
||
| mockedPut := &mockDoPutClient{} | ||
| defer mockedPut.AssertExpectations(s.T()) | ||
| mockedPut.On("Send", mock.MatchedBy(func(fd *flight.FlightData) bool { | ||
| return proto.Equal(updateDesc, fd.FlightDescriptor) | ||
| })).Return(nil) | ||
| mockedPut.On("CloseSend").Return(nil) | ||
| mockedPut.On("Recv").Return(&pb.PutResult{AppMetadata: resdata}, nil) | ||
| s.mockClient.On("DoPut", s.callOpts).Return(mockedPut, nil) | ||
|
|
||
| prepared, err := s.sqlClient.Prepare(context.TODO(), query, s.callOpts...) | ||
| s.NoError(err) | ||
| defer prepared.Close(context.TODO(), s.callOpts...) | ||
|
|
||
| s.Equal(string(prepared.Handle()), query) | ||
| val, ok := prepared.IsUpdate() | ||
| s.Require().True(ok) | ||
| s.True(val) | ||
|
|
||
| // Execute as update | ||
| num, err := prepared.ExecuteUpdate(context.TODO(), s.callOpts...) | ||
| s.NoError(err) | ||
| s.EqualValues(1, num) | ||
| } | ||
|
|
||
| func (s *FlightSqlClientSuite) TestPreparedStatementExecuteParamBinding() { | ||
| const query = "query" | ||
| const handle = "handle" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a defect, but this needs a release note, because the public API impact is wider than this struct.
type CreatePreparedStatementResult = pb.ActionCreatePreparedStatementResultBeing a type alias, the regenerated proto field lands on a public return flightsql.ActionCreatePreparedStatementResult{handle, ds, ps}, nilwill stop compiling. That's permitted under the Go 1 compatibility rules — unkeyed literals of imported struct types are explicitly not covered — and The unconditional |
||
| } | ||
|
|
||
| type ActionBeginTransactionRequest interface{} | ||
|
|
@@ -1251,6 +1254,7 @@ func (f *flightSqlServer) DoAction(cmd *flight.Action, stream flight.FlightServi | |
| if output.ParameterSchema != nil { | ||
| result.ParameterSchema = flight.SerializeSchema(output.ParameterSchema, f.mem) | ||
| } | ||
| result.IsUpdate = output.IsUpdate | ||
|
|
||
| if err := anycmd.MarshalFrom(&result); err != nil { | ||
| return status.Errorf(codes.Internal, "unable to marshal final response: %s", err.Error()) | ||
|
|
@@ -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) | ||
| } | ||
| result.IsUpdate = output.IsUpdate | ||
|
|
||
|
Comment on lines
1290
to
1294
|
||
| if err := anycmd.MarshalFrom(&result); err != nil { | ||
| return status.Errorf(codes.Internal, "unable to marshal final response: %s", err.Error()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 case "prepared no hint":
// leave result.IsUpdate nil - server provides no hintThen a short test asserting Thanks for wiring up
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed this in 221f7d6 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 The same nuance about checking |
||
| query := req.GetQuery() | ||
| 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) | ||
| } | ||
| return | ||
|
Comment on lines
+137
to
+150
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. This was also highlighted by Matt and I addressed it in #732 (comment) |
||
| } | ||
|
|
||
| func (*testServer) GetFlightInfoPreparedStatement(ctx context.Context, q flightsql.PreparedStatementQuery, fd *flight.FlightDescriptor) (*flight.FlightInfo, error) { | ||
| return &flight.FlightInfo{ | ||
| FlightDescriptor: fd, | ||
| Endpoint: []*flight.FlightEndpoint{{ | ||
| Ticket: &flight.Ticket{Ticket: fd.Cmd}, | ||
| }}, | ||
| }, nil | ||
| } | ||
|
|
||
| func (*testServer) DoGetPreparedStatement(ctx context.Context, q flightsql.PreparedStatementQuery) (sc *arrow.Schema, cc <-chan flight.StreamChunk, err error) { | ||
| handle := string(q.GetPreparedStatementHandle()) | ||
| switch handle { | ||
| case "prepared query": | ||
| b := array.NewInt16Builder(memory.DefaultAllocator) | ||
| sc = arrow.NewSchema([]arrow.Field{{ | ||
| Name: "t1", | ||
| Type: b.Type(), | ||
| Nullable: true, | ||
| }}, nil) | ||
| b.AppendNull() | ||
| c := make(chan flight.StreamChunk, 2) | ||
| c <- flight.StreamChunk{ | ||
| Data: array.NewRecordBatch(sc, []arrow.Array{b.NewArray()}, 1), | ||
| } | ||
| b.Append(1) | ||
| c <- flight.StreamChunk{ | ||
| Data: array.NewRecordBatch(sc, []arrow.Array{b.NewArray()}, 1), | ||
| } | ||
| close(c) | ||
| cc = c | ||
| default: | ||
| err = fmt.Errorf("unknown prepared statement handle: %s", handle) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func (*testServer) ClosePreparedStatement(ctx context.Context, req flightsql.ActionClosePreparedStatementRequest) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (*testServer) DoPutPreparedStatementUpdate(ctx context.Context, cmd flightsql.PreparedStatementUpdate, rdr flight.MessageReader) (int64, error) { | ||
| handle := string(cmd.GetPreparedStatementHandle()) | ||
| switch handle { | ||
| case "prepared update": | ||
| // Simulates executing a DML query | ||
| // that affects 2 rows | ||
| return 2, nil | ||
| default: | ||
| return 0, fmt.Errorf("unknown prepared statement handle: %s", handle) | ||
| } | ||
| } | ||
|
|
||
| func (*testServer) SetSessionOptions(ctx context.Context, req *flight.SetSessionOptionsRequest) (*flight.SetSessionOptionsResult, error) { | ||
| session, err := session.GetSessionFromContext(ctx) | ||
| if err != nil { | ||
|
|
@@ -260,9 +328,9 @@ func (s *FlightSqlServerSuite) TestExecute() { | |
| for i := 0; i < data.Len(); i++ { | ||
| switch n { | ||
| case 0: | ||
| s.Assert().Equal(true, data.IsNull(i)) | ||
| s.True(data.IsNull(i)) | ||
| case 1: | ||
| s.Assert().Equal(false, data.IsNull(i)) | ||
| s.False(data.IsNull(i)) | ||
| s.Assert().Equal(int16(1), data.Value(i)) | ||
| } | ||
| n++ | ||
|
|
@@ -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() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit / optional: the Substrait path ( Separately: the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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?
|
||
| prep, err := s.cl.Prepare(context.TODO(), "prepared update") | ||
| s.Require().NoError(err) | ||
| defer prep.Close(context.TODO()) | ||
|
|
||
| val, ok := prep.IsUpdate() | ||
| s.Require().True(ok) | ||
| s.True(val) | ||
|
|
||
| nrecords, err := prep.ExecuteUpdate(context.TODO()) | ||
| s.Require().NoError(err) | ||
| s.Assert().Equal(int64(2), nrecords) | ||
| } | ||
|
|
||
| func (s *FlightSqlServerSuite) TestExecutePoll() { | ||
| poll, err := s.cl.ExecutePoll(context.TODO(), "1", nil) | ||
| s.NoError(err) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small things on this propagation site.
Untested.
TestPreparedStatementLoadFromResult(client_test.go:971) is the only coverage forLoadPreparedStatementFromResult, and it doesn't assertIsUpdate. Since that test already constructs the result and checks the other two propagated fields (ParameterSchema,DatasetSchema), adding anIsUpdateassertion 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
*boolowned by the caller, so a caller mutatingresult.IsUpdateafter this call would change the statement's view.Prepare/PrepareSubstraitdon't have this property since they point at their own unmarshaled local. Not worth a defensive copy unless you'd want it forhandletoo — just noting it so the asymmetry is a conscious choice.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestPreparedStatementLoadFromResultI let things as they are, and tested with the new field unset, see 514d3c6. Note that this test has the same nuance aboutvalthat 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
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
handleandisUpdate? If so I can tackle that after this one