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 2023/09/11 14:38:13 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #1048: feat(go): add basic driver logging
lidavidm opened a new pull request, #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048
Fixes #492.
--
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] kou commented on a diff in pull request #1048: feat(go): add basic driver logging
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#discussion_r1322225599
##########
ci/linux-packages/debian/rules:
##########
@@ -31,17 +31,19 @@ export DEB_BUILD_MAINT_OPTIONS=reproducible=-timeless
CMAKE_BUILD_TYPE = RelWithDebInfo
override_dh_auto_configure:
- dh_auto_configure \
- --sourcedirectory=c/ \
- --builddirectory=c.build \
- --buildsystem=cmake+ninja \
- -- \
- -DCMAKE_BUILD_TYPE=$(CMAKE_BUILD_TYPE) \
- -DADBC_DRIVER_MANAGER=ON \
- -DADBC_DRIVER_POSTGRESQL=ON \
- -DADBC_DRIVER_SQLITE=ON \
- -DADBC_DRIVER_FLIGHTSQL=ON \
- -DADBC_DRIVER_SNOWFLAKE=ON
+ env \
+ PATH=$(PATH):/usr/lib/go-1.20/bin \
Review Comment:
I think that environment variable not Makefile variable is better here:
```suggestion
PATH=$${PATH}:/usr/lib/go-1.20/bin \
```
(`PATH=/usr/lib/go-1.20/bin:$${PATH}` may be better to ensure using Go 1.20 if available.)
--
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 pull request #1048: feat(go): add basic driver logging
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714542250
I didn't know there was an official package for go1.20 in the jammy repos
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714309389
It looks like slog works on 1.19 too so I'll see about bumping to 1.19 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 merged pull request #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714477430
Hmm, let's see if [conditional dependencies](https://www.debian.org/doc/debian-policy/ch-relationships.html#syntax-of-relationship-fields) are the right thing to do...
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714038265
It looks like Arrow itself was bumped to Go 1.20: https://github.com/apache/arrow/blob/main/go/go.mod
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#discussion_r1321904142
##########
r/adbcflightsql/tools/download-go.R:
##########
@@ -17,7 +17,7 @@
tmp_dir <- "src/go/tmp"
-go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.18.10")
+go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.19.13")
Review Comment:
We could use the `.env` but it's not in sync with everything else anyways (I guess it only affects docker-compose)
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714031088
@zeroshade how do you feel about bumping us to Go 1.20 or 1.21? I think the main problem will be Linux packaging jobs where we won't be able to use the system golang...
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714504988
Alright, now the problem is that the `golang-1.20` package doesn't add itself to 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
[GitHub] [arrow-adbc] lidavidm commented on pull request #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714567208
OK, so long as the apt changes look OK, I'll split the Go version bump into a new PR, then come back here once that's merged
--
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 pull request #1048: feat(go): add basic driver logging
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714536259
@lidavidm you should be able to use this PPA to install golang-1.20
```
sudo add-apt-repository ppa:longsleep/golang-backports
sudo apt update
sudo apt install golang-go
```
--
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 pull request #1048: feat(go): add basic driver logging
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714134519
Arrow *currently* maintains compatibility with go1.17-go1.20. But as of https://github.com/apache/arrow/pull/37637 we're dropping explicit compatibility with go1.17-go1.18. We're still keeping compatibility with go1.19 though, using build tags.
What we could do here is put together a small interface and then use build tags to use https://pkg.go.dev/golang.org/x/exp/slog for go1.19 and `slog` directly for go1.20+ similar to how we handle json in Arrow for tinygo vs non-tinygo
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714789901
Thanks Kou! Split into https://github.com/apache/arrow-adbc/pull/1053
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714031874
Either that, or we can do build tag shenanigans to only enable this on Go 1.20?
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714364941
hmm, ubuntu-jammy fails because we install `golang-1.20` but the package depends on `golang`. Let's see if we can finagle 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
[GitHub] [arrow-adbc] lidavidm commented on pull request #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714419149
Windows verification is failing since libprotobuf now has a runtime dependency on gtest, and so when our verification script tries to remove it (since we want to use bundled gtest on windows) it also clobbers half our conda environment
https://github.com/conda-forge/libprotobuf-feedstock/issues/186
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714351608
Once I get this working I'll split out the version bump into a separate PR so that it's clear
--
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] paleolimbot commented on a diff in pull request #1048: feat(go): add basic driver logging
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#discussion_r1321902096
##########
r/adbcflightsql/tools/download-go.R:
##########
@@ -17,7 +17,7 @@
tmp_dir <- "src/go/tmp"
-go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.18.10")
+go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.19.13")
Review Comment:
Is there a canonical place in the repository where this value could be scraped? I am pretty sure I can make this automatic so that you don't have to do the version bump every time.
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714425364
@kou do you know how to install `golang-1.20` on Ubuntu Jammy and then have the APT package build be OK with having that installed in place of `golang`?
I also tried installing both `golang` and `golang-1.20`, but then go 1.18 ends up first on PATH and I couldn't get update-alternatives to work with go.
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714539724
I don't think we want to ship binaries that depend on a PPA (and Go 1.20 is in the repos, just have to finagle the configuration)
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714549411
https://packages.ubuntu.com/jammy-updates/golang-1.20-go
--
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 #1048: feat(go): add basic driver logging
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1048:
URL: https://github.com/apache/arrow-adbc/pull/1048#issuecomment-1714030028
Bah, the experimental slog requires Go 1.20?
--
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