You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ywc88 (via GitHub)" <gi...@apache.org> on 2023/08/16 16:52:30 UTC
[GitHub] [arrow-adbc] ywc88 opened a new pull request, #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
ywc88 opened a new pull request, #980:
URL: https://github.com/apache/arrow-adbc/pull/980
Fixes #934.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm merged pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#issuecomment-1688632162
BTW @zeroshade, one thing I noticed while debugging this PR:
https://github.com/apache/arrow/blob/702e9cae138e8d372643e3545375d8fc4c6212a2/go/arrow/flight/flightsql/server.go#L832-L842
```go
for chunk := range cc {
if chunk.Err != nil {
return chunk.Err
}
wr.SetFlightDescriptor(chunk.Desc)
if err = wr.WriteWithAppMetadata(chunk.Data, chunk.AppMetadata); err != nil {
return err
}
chunk.Data.Release()
}
```
Shouldn't we release `chunk.Data` in both branches of the `if`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#discussion_r1301938459
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -1231,24 +1231,42 @@ func (c *cnxn) GetTableSchema(ctx context.Context, catalog *string, dbSchema *st
return nil, adbcFromFlightStatus(err, "GetTableSchema(DoGet)")
}
- if rec.NumRows() == 0 {
+ numRows := rec.NumRows()
+ if numRows == 0 {
return nil, adbc.Error{
Code: adbc.StatusNotFound,
}
}
+ if numRows > math.MaxInt32 {
+ return nil, adbc.Error{
+ Msg: "[Flight SQL] GetTableSchema cannot handle tables with number of rows > 2^31 - 1",
+ Code: adbc.StatusNotImplemented,
+ }
+ }
Review Comment:
From a stylistic perspective, let's use a switch
```go
switch {
case numRows == 0:
return.....
case numRows > math.MaxInt32:
return.....
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#discussion_r1301989020
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -1200,6 +1200,7 @@ func (c *cnxn) getObjectsTables(ctx context.Context, depth adbc.ObjectDepth, cat
return
}
+// Regression test for https://github.com/apache/arrow-adbc/issues/934
Review Comment:
Er, did you mean to put that here?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#discussion_r1301974454
##########
go/adbc/driver/flightsql/flightsql_adbc_server_test.go:
##########
@@ -627,3 +632,89 @@ func (suite *DataTypeTests) TestListInt() {
func (suite *DataTypeTests) TestMapIntInt() {
suite.DoTestCase("map[int]int", SchemaMapIntInt)
}
+
+// ---- Multi Table Tests --------------------
+
+type MultiTableTestServer struct {
+ flightsql.BaseServer
+}
+
+func (server *MultiTableTestServer) GetFlightInfoStatement(ctx context.Context, cmd flightsql.StatementQuery, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ query := cmd.GetQuery()
+ tkt, err := flightsql.CreateStatementQueryTicket([]byte(query))
+ if err != nil {
+ return nil, err
+ }
+
+ return &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{{Ticket: &flight.Ticket{Ticket: tkt}}},
+ FlightDescriptor: desc,
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }, nil
+}
+
+func (server *MultiTableTestServer) GetFlightInfoTables(ctx context.Context, cmd flightsql.GetTables, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ schema := schema_ref.Tables
+ if cmd.GetIncludeSchema() {
+ schema = schema_ref.TablesWithIncludedSchema
+ }
+ server.Alloc = memory.NewCheckedAllocator(memory.DefaultAllocator)
+ info := &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{
+ {Ticket: &flight.Ticket{Ticket: desc.Cmd}},
+ },
+ FlightDescriptor: desc,
+ Schema: flight.SerializeSchema(schema, server.Alloc),
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }
+
+ return info, nil
+}
+
+func (server *MultiTableTestServer) DoGetTables(ctx context.Context, cmd flightsql.GetTables) (*arrow.Schema, <-chan flight.StreamChunk, error) {
+ bldr := array.NewRecordBuilder(server.Alloc, adbc.GetTableSchemaSchema)
+
+ bldr.Field(0).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(1).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(2).(*array.StringBuilder).AppendValues([]string{"tbl1", "tbl2"}, nil)
Review Comment:
Ah, okay. So we literally just accepted whatever the first response was, no matter what...I see now, thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#discussion_r1301630844
##########
go/adbc/driver/flightsql/flightsql_adbc_server_test.go:
##########
@@ -627,3 +632,89 @@ func (suite *DataTypeTests) TestListInt() {
func (suite *DataTypeTests) TestMapIntInt() {
suite.DoTestCase("map[int]int", SchemaMapIntInt)
}
+
+// ---- Multi Table Tests --------------------
+
+type MultiTableTestServer struct {
+ flightsql.BaseServer
+}
+
+func (server *MultiTableTestServer) GetFlightInfoStatement(ctx context.Context, cmd flightsql.StatementQuery, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ query := cmd.GetQuery()
+ tkt, err := flightsql.CreateStatementQueryTicket([]byte(query))
+ if err != nil {
+ return nil, err
+ }
+
+ return &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{{Ticket: &flight.Ticket{Ticket: tkt}}},
+ FlightDescriptor: desc,
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }, nil
+}
+
+func (server *MultiTableTestServer) GetFlightInfoTables(ctx context.Context, cmd flightsql.GetTables, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ schema := schema_ref.Tables
+ if cmd.GetIncludeSchema() {
+ schema = schema_ref.TablesWithIncludedSchema
+ }
+ server.Alloc = memory.NewCheckedAllocator(memory.DefaultAllocator)
+ info := &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{
+ {Ticket: &flight.Ticket{Ticket: desc.Cmd}},
+ },
+ FlightDescriptor: desc,
+ Schema: flight.SerializeSchema(schema, server.Alloc),
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }
+
+ return info, nil
+}
+
+func (server *MultiTableTestServer) DoGetTables(ctx context.Context, cmd flightsql.GetTables) (*arrow.Schema, <-chan flight.StreamChunk, error) {
+ bldr := array.NewRecordBuilder(server.Alloc, adbc.GetTableSchemaSchema)
+
+ bldr.Field(0).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(1).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(2).(*array.StringBuilder).AppendValues([]string{"tbl1", "tbl2"}, nil)
+ bldr.Field(3).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+
+ sc1 := arrow.NewSchema([]arrow.Field{{Name: "a", Type: arrow.PrimitiveTypes.Int32, Nullable: true}}, nil)
+ sc2 := arrow.NewSchema([]arrow.Field{{Name: "b", Type: arrow.PrimitiveTypes.Int32, Nullable: true}}, nil)
+ buf1 := flight.SerializeSchema(sc1, server.Alloc)
+ buf2 := flight.SerializeSchema(sc2, server.Alloc)
+
+ bldr.Field(4).(*array.BinaryBuilder).AppendValues([][]byte{buf1, buf2}, nil)
+ defer bldr.Release()
+
+ rec := bldr.NewRecord()
+
+ ch := make(chan flight.StreamChunk)
+ go func() {
+ defer close(ch)
+ ch <- flight.StreamChunk{
+ Data: rec,
+ Desc: nil,
+ Err: nil,
+ }
+ }()
+ return adbc.GetTableSchemaSchema, ch, nil
+}
+
+type MultiTableTests struct {
+ ServerBasedTests
+}
+
+func (suite *MultiTableTests) SetupSuite() {
+ suite.DoSetupSuite(&MultiTableTestServer{}, nil, map[string]string{})
+}
+
+func (suite *MultiTableTests) TestGetTableSchema() {
Review Comment:
```suggestion
// Regression test for https://github.com/apache/arrow-adbc/issues/934
func (suite *MultiTableTests) TestGetTableSchema() {
```
##########
go/adbc/driver/flightsql/flightsql_adbc_server_test.go:
##########
@@ -627,3 +632,89 @@ func (suite *DataTypeTests) TestListInt() {
func (suite *DataTypeTests) TestMapIntInt() {
suite.DoTestCase("map[int]int", SchemaMapIntInt)
}
+
+// ---- Multi Table Tests --------------------
+
+type MultiTableTestServer struct {
+ flightsql.BaseServer
+}
+
+func (server *MultiTableTestServer) GetFlightInfoStatement(ctx context.Context, cmd flightsql.StatementQuery, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ query := cmd.GetQuery()
+ tkt, err := flightsql.CreateStatementQueryTicket([]byte(query))
+ if err != nil {
+ return nil, err
+ }
+
+ return &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{{Ticket: &flight.Ticket{Ticket: tkt}}},
+ FlightDescriptor: desc,
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }, nil
+}
+
+func (server *MultiTableTestServer) GetFlightInfoTables(ctx context.Context, cmd flightsql.GetTables, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ schema := schema_ref.Tables
+ if cmd.GetIncludeSchema() {
+ schema = schema_ref.TablesWithIncludedSchema
+ }
+ server.Alloc = memory.NewCheckedAllocator(memory.DefaultAllocator)
+ info := &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{
+ {Ticket: &flight.Ticket{Ticket: desc.Cmd}},
+ },
+ FlightDescriptor: desc,
+ Schema: flight.SerializeSchema(schema, server.Alloc),
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }
+
+ return info, nil
+}
+
+func (server *MultiTableTestServer) DoGetTables(ctx context.Context, cmd flightsql.GetTables) (*arrow.Schema, <-chan flight.StreamChunk, error) {
+ bldr := array.NewRecordBuilder(server.Alloc, adbc.GetTableSchemaSchema)
+
+ bldr.Field(0).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(1).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(2).(*array.StringBuilder).AppendValues([]string{"tbl1", "tbl2"}, nil)
Review Comment:
I don't think this tests the original bug, though, where the issue is that if you requested a table ("foo") and the server returned multiple tables where the requested table was a prefix of one of the requested tables ("foobar"), then the client would pick that table.
##########
go/adbc/driver/flightsql/flightsql_adbc_server_test.go:
##########
@@ -627,3 +632,89 @@ func (suite *DataTypeTests) TestListInt() {
func (suite *DataTypeTests) TestMapIntInt() {
suite.DoTestCase("map[int]int", SchemaMapIntInt)
}
+
+// ---- Multi Table Tests --------------------
+
+type MultiTableTestServer struct {
+ flightsql.BaseServer
+}
+
+func (server *MultiTableTestServer) GetFlightInfoStatement(ctx context.Context, cmd flightsql.StatementQuery, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ query := cmd.GetQuery()
+ tkt, err := flightsql.CreateStatementQueryTicket([]byte(query))
+ if err != nil {
+ return nil, err
+ }
+
+ return &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{{Ticket: &flight.Ticket{Ticket: tkt}}},
+ FlightDescriptor: desc,
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }, nil
+}
+
+func (server *MultiTableTestServer) GetFlightInfoTables(ctx context.Context, cmd flightsql.GetTables, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ schema := schema_ref.Tables
+ if cmd.GetIncludeSchema() {
+ schema = schema_ref.TablesWithIncludedSchema
+ }
+ server.Alloc = memory.NewCheckedAllocator(memory.DefaultAllocator)
+ info := &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{
+ {Ticket: &flight.Ticket{Ticket: desc.Cmd}},
+ },
+ FlightDescriptor: desc,
+ Schema: flight.SerializeSchema(schema, server.Alloc),
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }
+
+ return info, nil
+}
+
+func (server *MultiTableTestServer) DoGetTables(ctx context.Context, cmd flightsql.GetTables) (*arrow.Schema, <-chan flight.StreamChunk, error) {
+ bldr := array.NewRecordBuilder(server.Alloc, adbc.GetTableSchemaSchema)
+
+ bldr.Field(0).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(1).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(2).(*array.StringBuilder).AppendValues([]string{"tbl1", "tbl2"}, nil)
Review Comment:
Does this test fail without the fix above?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] ywc88 commented on a diff in pull request #980: fix(go/adbc/driver/flightsql): Have GetTableSchema check for table name match instead of the first schema it receives
Posted by "ywc88 (via GitHub)" <gi...@apache.org>.
ywc88 commented on code in PR #980:
URL: https://github.com/apache/arrow-adbc/pull/980#discussion_r1301971921
##########
go/adbc/driver/flightsql/flightsql_adbc_server_test.go:
##########
@@ -627,3 +632,89 @@ func (suite *DataTypeTests) TestListInt() {
func (suite *DataTypeTests) TestMapIntInt() {
suite.DoTestCase("map[int]int", SchemaMapIntInt)
}
+
+// ---- Multi Table Tests --------------------
+
+type MultiTableTestServer struct {
+ flightsql.BaseServer
+}
+
+func (server *MultiTableTestServer) GetFlightInfoStatement(ctx context.Context, cmd flightsql.StatementQuery, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ query := cmd.GetQuery()
+ tkt, err := flightsql.CreateStatementQueryTicket([]byte(query))
+ if err != nil {
+ return nil, err
+ }
+
+ return &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{{Ticket: &flight.Ticket{Ticket: tkt}}},
+ FlightDescriptor: desc,
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }, nil
+}
+
+func (server *MultiTableTestServer) GetFlightInfoTables(ctx context.Context, cmd flightsql.GetTables, desc *flight.FlightDescriptor) (*flight.FlightInfo, error) {
+ schema := schema_ref.Tables
+ if cmd.GetIncludeSchema() {
+ schema = schema_ref.TablesWithIncludedSchema
+ }
+ server.Alloc = memory.NewCheckedAllocator(memory.DefaultAllocator)
+ info := &flight.FlightInfo{
+ Endpoint: []*flight.FlightEndpoint{
+ {Ticket: &flight.Ticket{Ticket: desc.Cmd}},
+ },
+ FlightDescriptor: desc,
+ Schema: flight.SerializeSchema(schema, server.Alloc),
+ TotalRecords: -1,
+ TotalBytes: -1,
+ }
+
+ return info, nil
+}
+
+func (server *MultiTableTestServer) DoGetTables(ctx context.Context, cmd flightsql.GetTables) (*arrow.Schema, <-chan flight.StreamChunk, error) {
+ bldr := array.NewRecordBuilder(server.Alloc, adbc.GetTableSchemaSchema)
+
+ bldr.Field(0).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(1).(*array.StringBuilder).AppendValues([]string{"", ""}, nil)
+ bldr.Field(2).(*array.StringBuilder).AppendValues([]string{"tbl1", "tbl2"}, nil)
Review Comment:
This test does fail without the fix:
```
go test flightsql_adbc_server_test.go -v -run TestMultiTable
=== RUN TestMultiTable
=== RUN TestMultiTable/TestGetTableSchema
flightsql_adbc_server_test.go:719:
Error Trace: /Users/ywc/Voltron Data/ywc88/arrow-adbc/go/adbc/driver/flightsql/flightsql_adbc_server_test.go:719
Error: Not equal:
expected: &arrow.Schema{fields:[]arrow.Field{arrow.Field{Name:"b", Type:(*arrow.Int32Type)(0x1057ab240), Nullable:true, Metadata:arrow.Metadata{keys:[]string(nil), values:[]string(nil)}}}, index:map[string][]int{"b":[]int{0}}, meta:arrow.Metadata{keys:[]string(nil), values:[]string(nil)}, endianness:0}
actual : &arrow.Schema{fields:[]arrow.Field{arrow.Field{Name:"a", Type:(*arrow.Int32Type)(0x1057ab240), Nullable:true, Metadata:arrow.Metadata{keys:[]string(nil), values:[]string(nil)}}}, index:map[string][]int{"a":[]int{0}}, meta:arrow.Metadata{keys:[]string(nil), values:[]string(nil)}, endianness:0}
Diff:
--- Expected
+++ Actual
@@ -3,3 +3,3 @@
(arrow.Field) {
- Name: (string) (len=1) "b",
+ Name: (string) (len=1) "a",
Type: (*arrow.Int32Type)({
@@ -14,3 +14,3 @@
index: (map[string][]int) (len=1) {
- (string) (len=1) "b": ([]int) (len=1) {
+ (string) (len=1) "a": ([]int) (len=1) {
(int) 0
Test: TestMultiTable/TestGetTableSchema
--- FAIL: TestMultiTable (0.01s)
--- FAIL: TestMultiTable/TestGetTableSchema (0.00s)
FAIL
FAIL command-line-arguments 0.194s
FAIL
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org