You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "joellubi (via GitHub)" <gi...@apache.org> on 2024/04/06 22:37:38 UTC
[PR] feat(go/adbc/drivermgr): Implement SetSubstraitPlan in CGO Wrapper and Add DuckDB to Test [arrow-adbc]
joellubi opened a new pull request, #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709
(no comment)
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on code in PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#discussion_r1559957138
##########
go/adbc/drivermgr/wrapper_duckdb_test.go:
##########
@@ -0,0 +1,163 @@
+// 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.
+
+//go:build cgo
+
+package drivermgr_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow-adbc/go/adbc/drivermgr"
+ "github.com/apache/arrow/go/v16/arrow"
+ "github.com/apache/arrow/go/v16/arrow/array"
+ "github.com/stretchr/testify/suite"
+ "github.com/substrait-io/substrait-go/extensions"
+ "github.com/substrait-io/substrait-go/plan"
+ "github.com/substrait-io/substrait-go/types"
+ "google.golang.org/protobuf/proto"
+)
+
+type DriverMgrDuckDBSuite struct {
+ suite.Suite
+
+ ctx context.Context
+ drv adbc.Driver
+ db adbc.Database
+ conn adbc.Connection
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupSuite() {
+ dm.ctx = context.TODO()
+ dm.drv = drivermgr.Driver{}
+ var err error
+ dm.db, err = dm.drv.NewDatabase(map[string]string{
+ "driver": "libduckdb",
+ "entrypoint": "duckdb_adbc_init",
+ })
+ dm.NoError(err)
+
+ cnxn, err := dm.db.Open(dm.ctx)
+ dm.NoError(err)
+ defer cnxn.Close()
+
+ stmt, err := cnxn.NewStatement()
+ dm.NoError(err)
+ defer stmt.Close()
+
+ // Setup substrait extension
+ dm.NoError(stmt.SetSqlQuery("INSTALL substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("LOAD substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("CREATE TABLE test_table (id INTEGER PRIMARY KEY, name TEXT)"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("INSERT INTO test_table (id, name) VALUES (1, 'test')"))
Review Comment:
I updated this to use a single statement as you suggested. I think I was just rushing lol.
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2041638826
> It looks like the issue is purely that conda isn't finding `libduckdb` for windows which is odd....
because on windows, it's probably duckdb.dll; Windows doesn't use the lib prefix. Use just "duckdb" and [on Windows it'll add .dll for you](https://github.com/apache/arrow-adbc/blob/dff48ec3d6a1ab0a81811600358ec0144392a8df/c/driver_manager/adbc_driver_manager.cc#L116-L128) and [otherwise it'll prepend lib and append .so/.dylib](https://github.com/apache/arrow-adbc/blob/dff48ec3d6a1ab0a81811600358ec0144392a8df/c/driver_manager/adbc_driver_manager.cc#L149-L166)
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#discussion_r1555048355
##########
go/adbc/drivermgr/wrapper_duckdb_test.go:
##########
@@ -0,0 +1,163 @@
+// 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.
+
+//go:build cgo
+
+package drivermgr_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow-adbc/go/adbc/drivermgr"
+ "github.com/apache/arrow/go/v16/arrow"
+ "github.com/apache/arrow/go/v16/arrow/array"
+ "github.com/stretchr/testify/suite"
+ "github.com/substrait-io/substrait-go/extensions"
+ "github.com/substrait-io/substrait-go/plan"
+ "github.com/substrait-io/substrait-go/types"
+ "google.golang.org/protobuf/proto"
+)
+
+type DriverMgrDuckDBSuite struct {
+ suite.Suite
+
+ ctx context.Context
+ drv adbc.Driver
+ db adbc.Database
+ conn adbc.Connection
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupSuite() {
+ dm.ctx = context.TODO()
+ dm.drv = drivermgr.Driver{}
+ var err error
+ dm.db, err = dm.drv.NewDatabase(map[string]string{
+ "driver": "libduckdb",
+ "entrypoint": "duckdb_adbc_init",
+ })
+ dm.NoError(err)
+
+ cnxn, err := dm.db.Open(dm.ctx)
+ dm.NoError(err)
+ defer cnxn.Close()
+
+ stmt, err := cnxn.NewStatement()
+ dm.NoError(err)
+ defer stmt.Close()
+
+ // Setup substrait extension
+ dm.NoError(stmt.SetSqlQuery("INSTALL substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("LOAD substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("CREATE TABLE test_table (id INTEGER PRIMARY KEY, name TEXT)"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("INSERT INTO test_table (id, name) VALUES (1, 'test')"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("INSERT INTO test_table (id, name) VALUES (2, 'test')"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("INSERT INTO test_table (id, name) VALUES (3, 'tested')"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+}
+
+func (dm *DriverMgrDuckDBSuite) TearDownSuite() {
+ dm.NoError(dm.db.Close())
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupTest() {
+ cnxn, err := dm.db.Open(dm.ctx)
+ dm.Require().NoError(err)
+ dm.conn = cnxn
+}
+
+func (dm *DriverMgrDuckDBSuite) TearDownTest() {
+ dm.NoError(dm.conn.Close())
+}
+
+func (dm *DriverMgrDuckDBSuite) TestSubstraitExecute() {
+ // Building Substrait plan equivalent to 'SELECT COUNT(*) AS count FROM test_table;'
+ bldr := plan.NewBuilderDefault()
+ scan := bldr.NamedScan(
+ []string{"test_table"},
+ types.NamedStruct{
+ Names: []string{"id", "names"},
+ Struct: types.StructType{
+ Nullability: types.NullabilityRequired,
+ Types: []types.Type{
+ &types.Int32Type{Nullability: types.NullabilityRequired},
+ &types.StringType{Nullability: types.NullabilityNullable},
+ },
+ },
+ },
+ )
+
+ countFn, err := bldr.AggregateFn(extensions.SubstraitDefaultURIPrefix+"functions_aggregate_generic.yaml", "count", nil)
+ dm.Require().NoError(err)
Review Comment:
*sigh* i should really improve this in the substrait library lol
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2048286439
> Hmm, but duckdb should be in the Conda env. Possibly add $CONDA_PREFIX/lib explicitly to DYLD_LIBRARY_PATH?
That did the trick. I think previously we were using libraries that were either built as part of the workflow or included in the distribution itself (I think this may have been the case with sqlite).
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2108549481
I rebased and reran the workflows with the newer release of duckdb. The test failures resolved as expected. The issue remains however that there is unfortunately no libduckdb package published to conda-forge for windows...
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2046060267
@lidavidm @zeroshade
As I looked more closely I realized it's not only Windows failing but also Unix CGO (e.g. macos-14). Is there anything that needs to be done to properly locate the library after installing with conda? I didn't see libsqlite getting copied into another directory, so I wasn't sure why only duckdb wouldn't work the same way. Here's the test failure:
```
=== RUN TestDriverMgrDuckDB
wrapper_duckdb_test.go:54:
Error Trace: /Users/runner/work/arrow-adbc/arrow-adbc/go/adbc/drivermgr/wrapper_duckdb_test.go:54
/Users/runner/go/pkg/mod/github.com/stretchr/testify@v1.9.0/suite/suite.go:157
/Users/runner/work/arrow-adbc/arrow-adbc/go/adbc/drivermgr/wrapper_duckdb_test.go:162
Error: Received unexpected error:
Internal: [Driver Manager] [DriverManager] dlopen() failed: dlopen(duckdb, 0x0006): tried: '/duckdb' (no such file), '/Users/runner/local/lib/duckdb' (no such file), 'duckdb' (no such file), '/System/Volumes/Preboot/Cryptexes/OSduckdb' (no such file), '/usr/lib/duckdb' (no such file, not in dyld cache), 'duckdb' (no such file)
dlopen() failed: dlopen(libduckdb.dylib, 0x0006): tried: '/libduckdb.dylib' (no such file), '/Users/runner/local/lib/libduckdb.dylib' (no such file), 'libduckdb.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OSlibduckdb.dylib' (no such file), '/usr/lib/libduckdb.dylib' (no such file, not in dyld cache), 'libduckdb.dylib' (no such file)
```
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#discussion_r1555103853
##########
go/adbc/drivermgr/wrapper_duckdb_test.go:
##########
@@ -0,0 +1,163 @@
+// 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.
+
+//go:build cgo
+
+package drivermgr_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow-adbc/go/adbc/drivermgr"
+ "github.com/apache/arrow/go/v16/arrow"
+ "github.com/apache/arrow/go/v16/arrow/array"
+ "github.com/stretchr/testify/suite"
+ "github.com/substrait-io/substrait-go/extensions"
+ "github.com/substrait-io/substrait-go/plan"
+ "github.com/substrait-io/substrait-go/types"
+ "google.golang.org/protobuf/proto"
+)
+
+type DriverMgrDuckDBSuite struct {
+ suite.Suite
+
+ ctx context.Context
+ drv adbc.Driver
+ db adbc.Database
+ conn adbc.Connection
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupSuite() {
+ dm.ctx = context.TODO()
+ dm.drv = drivermgr.Driver{}
+ var err error
+ dm.db, err = dm.drv.NewDatabase(map[string]string{
+ "driver": "libduckdb",
Review Comment:
I think you should prefer just "duckdb", and the C++ driver manger will look for "libduckdb.so" and "duckdb.dll"
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2046139590
Hmm, but duckdb should be in the Conda env. Possibly add $CONDA_PREFIX/lib explicitly to DYLD_LIBRARY_PATH?
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2041667242
If someone knows the right things to do here (I outlined what I think is necessary but don't really have the time: need to add the mingw gcc on Windows + configure the Go builds here to use a different compiler than CC/CXX) https://github.com/conda-forge/arrow-adbc-split-feedstock/pull/17
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2041667513
I think it's fine to download nightly duckdb/in the future we could even test against both in the workflow (just repeat the steps)
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#discussion_r1555047716
##########
go/adbc/drivermgr/wrapper_duckdb_test.go:
##########
@@ -0,0 +1,163 @@
+// 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.
+
+//go:build cgo
+
+package drivermgr_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow-adbc/go/adbc/drivermgr"
+ "github.com/apache/arrow/go/v16/arrow"
+ "github.com/apache/arrow/go/v16/arrow/array"
+ "github.com/stretchr/testify/suite"
+ "github.com/substrait-io/substrait-go/extensions"
+ "github.com/substrait-io/substrait-go/plan"
+ "github.com/substrait-io/substrait-go/types"
+ "google.golang.org/protobuf/proto"
+)
+
+type DriverMgrDuckDBSuite struct {
+ suite.Suite
+
+ ctx context.Context
+ drv adbc.Driver
+ db adbc.Database
+ conn adbc.Connection
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupSuite() {
+ dm.ctx = context.TODO()
+ dm.drv = drivermgr.Driver{}
+ var err error
+ dm.db, err = dm.drv.NewDatabase(map[string]string{
+ "driver": "libduckdb",
Review Comment:
i'm honestly surprised this works without needing the suffix. That's really cool to know and can likely help us simplify some other CI
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2041659556
> > It looks like the issue is purely that conda isn't finding `libduckdb` for windows which is odd....
>
> because on windows, it's probably duckdb.dll; Windows doesn't use the lib prefix. Use just "duckdb" and [on Windows it'll add .dll for you](https://github.com/apache/arrow-adbc/blob/dff48ec3d6a1ab0a81811600358ec0144392a8df/c/driver_manager/adbc_driver_manager.cc#L116-L128) and [otherwise it'll prepend lib and append .so/.dylib](https://github.com/apache/arrow-adbc/blob/dff48ec3d6a1ab0a81811600358ec0144392a8df/c/driver_manager/adbc_driver_manager.cc#L149-L166)
I tried just `duckdb` without `lib` and it works, which is nice. I think there's another issue though which is that there doesn't seem to even be a windows version hosted on [conda-forge](https://anaconda.org/conda-forge/libduckdb)...
That said, I had been testing this with a [nightly release](https://duckdb.org/docs/installation/?version=main&environment=cplusplus&platform=linux&architecture=x86_64) of duckdb because some substrait issues were recently fixed but hadn't yet been included in a stable release. Testing against nightly might help surface issues/drift early. I suppose I could just download those in the github workflow and branch on the os / arch. Kind of complicated unfortunately. What do you think?
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#discussion_r1555047906
##########
go/adbc/drivermgr/wrapper_duckdb_test.go:
##########
@@ -0,0 +1,163 @@
+// 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.
+
+//go:build cgo
+
+package drivermgr_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-adbc/go/adbc"
+ "github.com/apache/arrow-adbc/go/adbc/drivermgr"
+ "github.com/apache/arrow/go/v16/arrow"
+ "github.com/apache/arrow/go/v16/arrow/array"
+ "github.com/stretchr/testify/suite"
+ "github.com/substrait-io/substrait-go/extensions"
+ "github.com/substrait-io/substrait-go/plan"
+ "github.com/substrait-io/substrait-go/types"
+ "google.golang.org/protobuf/proto"
+)
+
+type DriverMgrDuckDBSuite struct {
+ suite.Suite
+
+ ctx context.Context
+ drv adbc.Driver
+ db adbc.Database
+ conn adbc.Connection
+}
+
+func (dm *DriverMgrDuckDBSuite) SetupSuite() {
+ dm.ctx = context.TODO()
+ dm.drv = drivermgr.Driver{}
+ var err error
+ dm.db, err = dm.drv.NewDatabase(map[string]string{
+ "driver": "libduckdb",
+ "entrypoint": "duckdb_adbc_init",
+ })
+ dm.NoError(err)
+
+ cnxn, err := dm.db.Open(dm.ctx)
+ dm.NoError(err)
+ defer cnxn.Close()
+
+ stmt, err := cnxn.NewStatement()
+ dm.NoError(err)
+ defer stmt.Close()
+
+ // Setup substrait extension
+ dm.NoError(stmt.SetSqlQuery("INSTALL substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("LOAD substrait"))
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("CREATE TABLE test_table (id INTEGER PRIMARY KEY, name TEXT)"))
+ // Don't check nrows, duckdb currently does not support it
+ _, err = stmt.ExecuteUpdate(dm.ctx)
+ dm.NoError(err)
+
+ dm.NoError(stmt.SetSqlQuery("INSERT INTO test_table (id, name) VALUES (1, 'test')"))
Review Comment:
any reason to not just insert all three rows in one query?
`INSERT INTO test_table (id, name) VALUES (1, 'test'), (2, 'test'), (3, 'tested')` or does DuckDB not support that?
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2041580320
Thanks so much for this, this is great both for adding the CI using duckdb and for SetSubtraitPlan being used.
It looks like the issue is purely that conda isn't finding `libduckdb` for windows which is odd....
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2046139008
I think we install into `/Users/runner/local/lib/` and set DYLD_LIBRARY_PATH when we build the SQLite driver, so we'd need to make sure DuckDB ends up in the same path.
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2048530212
Cool, thanks for investigating.
--
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/drivermgr): Implement SetSubstraitPlan in CGO wrapper and add DuckDB wrapper tests [arrow-adbc]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #1709:
URL: https://github.com/apache/arrow-adbc/pull/1709#issuecomment-2048297217
I looked into running the workflow against nightly duckdb...
While I think it would be nice to test against nightlies, they are not currently uploaded to any package manager. The files need to be downloaded and unzipped ([link](https://duckdb.org/docs/installation/index?version=main&environment=cplusplus&platform=linux)). This isn't too bad but the naming and paths aren't very consistent across platforms and architectures, so while we could get this working now it will very likely be a brittle workflow if/when the names/paths change in the future.
According to the [release calendar](https://duckdb.org/docs/dev/release_calendar), an official release including the required substrait fixes should be available within a week or so. In the interest of simplicity, I think we can just leave this PR as a draft and re-run the workflow in a week to confirm the tests pass.
--
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