You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2024/05/03 01:57:09 UTC
[PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
lidavidm opened a new pull request, #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808
Fixes #1144.
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2093906470
Hmm.
```
=================================================================
==8722==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 80 byte(s) in 1 object(s) allocated from:
#0 0x7fa643a955f8 in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x7fa642104139 in get_arr /home/runner/go/pkg/mod/github.com/apache/arrow/go/v17@v17.0.0-20240503231747-7cd9c6fbd313/arrow/cdata/cdata.go:31
#2 0x7fa6421042d3 in _cgo_7084d99dab99_Cfunc_get_arr /tmp/go-build/cgo-gcc-prolog:144
#3 0x7fa6406f8884 in runtime.asmcgocall /opt/hostedtoolcache/go/1.21.8/x64/src/runtime/asm_amd64.s:872
Indirect leak of 584 byte(s) in 4 object(s) allocated from:
#0 0x7fa643a955f8 in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x5586872db7b0 in PrivateArrowMalloc /home/runner/work/arrow-adbc/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:186
Indirect leak of 33 byte(s) in 2 object(s) allocated from:
#0 0x7fa643a94dd2 in __interceptor_realloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:85
#1 0x5586872db7bf in PrivateArrowRealloc /home/runner/work/arrow-adbc/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:188
SUMMARY: AddressSanitizer: 697 byte(s) leaked in 7 allocation(s).
```
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#discussion_r1591627339
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
Review Comment:
I'm opting to punt on most types for now, yeah.
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2093344235
I tracked down the cause of the leak, it's not a GC issue. I filed an upstream PR to fix the leak in the cdata package of Go Arrow which is linked 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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2092362135
@zeroshade could you help figure out the ASAN error? Reducing SqlPrepareUpdateStream to this first part still seems to trigger it, so to me it seems like a GC issue again:
```
void StatementTest::TestSqlPrepareUpdateStream() {
if (!quirks()->supports_bulk_ingest(ADBC_INGEST_OPTION_MODE_CREATE) ||
!quirks()->supports_dynamic_parameter_binding()) {
GTEST_SKIP();
}
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
ASSERT_THAT(quirks()->DropTable(&connection, "bulk_ingest", &error),
IsOkStatus(&error));
struct ArrowError na_error;
const std::vector<SchemaField> fields = {{"ints", NANOARROW_TYPE_INT64}};
// Create table
{
Handle<struct ArrowSchema> schema;
Handle<struct ArrowArray> array;
ASSERT_THAT(AdbcStatementSetOption(&statement, ADBC_INGEST_OPTION_TARGET_TABLE,
"bulk_ingest", &error),
IsOkStatus(&error));
ASSERT_THAT(MakeSchema(&schema.value, fields), IsOkErrno());
ASSERT_THAT((MakeBatch<int64_t>(&schema.value, &array.value, &na_error, {})),
IsOkErrno(&na_error));
ASSERT_THAT(AdbcStatementBind(&statement, &array.value, &schema.value, &error),
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
IsOkStatus(&error));
}
```
```
Direct leak of 160 byte(s) in 2 object(s) allocated from:
#0 0x7f50bac9561c in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x7f50b95c1339 in get_arr /home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/v17@v17.0.0-20240430043840-e4f31462dbd6/arrow/cdata/cdata.go:31
#2 0x7f50b95c14d3 in _cgo_1f0435ccdf41_Cfunc_get_arr /tmp/go-build/cgo-gcc-prolog:144
#3 0x7f50b7bb64e4 in runtime.asmcgocall /usr/lib/go-1.21/src/runtime/asm_amd64.s:872
Indirect leak of 584 byte(s) in 4 object(s) allocated from:
#0 0x7f50bac9561c in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x55763b631bab in PrivateArrowMalloc /home/lidavidm/Code/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:186
Indirect leak of 33 byte(s) in 2 object(s) allocated from:
#0 0x7f50bac94df6 in __interceptor_realloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:85
#1 0x55763b631bba in PrivateArrowRealloc /home/lidavidm/Code/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:188
SUMMARY: AddressSanitizer: 777 byte(s) leaked in 8 allocation(s).
```
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#discussion_r1591109804
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
Review Comment:
do we want to also support lists / snowflake's ARRAY type? or struct / snowflake's Object type by JSON marshalling to a string? we could also just punt on that until someone asks for it (probably the better choice right now)
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
Review Comment:
Something for us to think about is that go1.22 added `sql.Null[T]` which might be useful to allow us to create a more generic implementation of this later on (since we officially still support go 1.21, we wouldn't be able to use it yet)
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
+ if r.stream != nil {
+ r.stream.Release()
+ }
+}
+
+func (r *snowflakeBindReader) Next() (array.RecordReader, error) {
+ params, err := r.NextParams()
+ if err != nil {
+ return nil, err
+ } else if params == nil {
Review Comment:
no need for `else` since we're returning in the previous condition
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
+ if r.stream != nil {
+ r.stream.Release()
+ }
+}
+
+func (r *snowflakeBindReader) Next() (array.RecordReader, error) {
+ params, err := r.NextParams()
+ if err != nil {
+ return nil, err
+ } else if params == nil {
+ // end-of-stream
+ return nil, nil
Review Comment:
`nil` defines end-of-stream instead of using `io.EOF`?
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
Review Comment:
I hate this, but nothing we can do right now i guess
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
Review Comment:
we might still end up with a double release here because we don't set `r.currentBatch` to `nil` after calling `stream.Next()` and getting the end of the stream. we should probably just always set `r.currentBatch = nil` before we call `stream.Next()` to ensure we don't double release. You can test for excess releases by running with `-tags assert` which will turn on the debug asserts in Arrow
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,141 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
+ if r.stream != nil {
+ r.stream.Release()
+ }
Review Comment:
we should probably also set both `r.currentBatch` and `r.stream` to `nil` during release, just to prevent issues if release is called multiple times somehow
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#discussion_r1589429926
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,144 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
+ if r.stream != nil {
+ r.stream.Release()
+ }
+}
+
+func (r *snowflakeBindReader) Next() (array.RecordReader, error) {
+ params, err := r.NextParams()
+ if err != nil {
+ return nil, err
+ } else if params == nil {
+ // end-of-stream
+ return nil, nil
+ }
+ return r.doQuery(params)
+}
+
+func (r *snowflakeBindReader) NextParams() ([]driver.NamedValue, error) {
+ for r.currentBatch == nil || r.nextIndex >= r.currentBatch.NumRows() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
Review Comment:
this isn't necessary, calling `r.stream.Next()` will automatically release the existing record because we never call retain 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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2093910564
Still?? Wtf?
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2094136424
I think I tracked it down to a missing release in one case, let's see :P
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#issuecomment-2095311755
@zeroshade can I just get a final look here once you're up?
--
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
Re: [PR] feat(go/adbc/driver/snowflake): support parameter binding [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1808:
URL: https://github.com/apache/arrow-adbc/pull/1808#discussion_r1589429926
##########
go/adbc/driver/snowflake/binding.go:
##########
@@ -0,0 +1,144 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package snowflake
+
+import (
+ "database/sql"
+ "database/sql/driver"
+ "fmt"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow/go/v17/arrow"
+ "github.com/apache/arrow/go/v17/arrow/array"
+)
+
+func convertArrowToNamedValue(batch arrow.Record, index int) ([]driver.NamedValue, error) {
+ // see goTypeToSnowflake in gosnowflake
+ // technically, snowflake can bind an array of values at once, but
+ // only for INSERT, so we can't take advantage of that without
+ // analyzing the query ourselves
+ params := make([]driver.NamedValue, batch.NumCols())
+ for i, field := range batch.Schema().Fields() {
+ rawColumn := batch.Column(i)
+ params[i].Ordinal = i + 1
+ switch column := rawColumn.(type) {
+ case *array.Boolean:
+ params[i].Value = sql.NullBool{
+ Bool: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float32:
+ // Snowflake only recognizes float64
+ params[i].Value = sql.NullFloat64{
+ Float64: float64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Float64:
+ params[i].Value = sql.NullFloat64{
+ Float64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int8:
+ // Snowflake only recognizes int64
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int16:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int32:
+ params[i].Value = sql.NullInt64{
+ Int64: int64(column.Value(index)),
+ Valid: column.IsValid(index),
+ }
+ case *array.Int64:
+ params[i].Value = sql.NullInt64{
+ Int64: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.String:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ case *array.LargeString:
+ params[i].Value = sql.NullString{
+ String: column.Value(index),
+ Valid: column.IsValid(index),
+ }
+ default:
+ return nil, adbc.Error{
+ Code: adbc.StatusNotImplemented,
+ Msg: fmt.Sprintf("[Snowflake] Unsupported bind param '%s' type %s", field.Name, field.Type.String()),
+ }
+ }
+ }
+ return params, nil
+}
+
+type snowflakeBindReader struct {
+ doQuery func([]driver.NamedValue) (array.RecordReader, error)
+ currentBatch arrow.Record
+ nextIndex int64
+ // may be nil if we bound only a batch
+ stream array.RecordReader
+}
+
+func (r *snowflakeBindReader) Release() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
+ if r.stream != nil {
+ r.stream.Release()
+ }
+}
+
+func (r *snowflakeBindReader) Next() (array.RecordReader, error) {
+ params, err := r.NextParams()
+ if err != nil {
+ return nil, err
+ } else if params == nil {
+ // end-of-stream
+ return nil, nil
+ }
+ return r.doQuery(params)
+}
+
+func (r *snowflakeBindReader) NextParams() ([]driver.NamedValue, error) {
+ for r.currentBatch == nil || r.nextIndex >= r.currentBatch.NumRows() {
+ if r.currentBatch != nil {
+ r.currentBatch.Release()
+ }
Review Comment:
this isn't necessary, calling `r.stream.Next()` will automatically release the existing record because we never call retain here. In the current form we're gonna end up with too many releases called on the batches.
--
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