-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(stdlib/sql): add BigQuery support #2925
Conversation
a370aac
to
f036833
Compare
Codecov Report
@@ Coverage Diff @@
## master #2925 +/- ##
==========================================
+ Coverage 51.75% 52.06% +0.31%
==========================================
Files 332 333 +1
Lines 41398 41502 +104
==========================================
+ Hits 21426 21610 +184
+ Misses 17511 17416 -95
- Partials 2461 2476 +15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is very fine work, as usual. I just had some questions below.
@@ -38,7 +40,7 @@ require ( | |||
github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 // indirect | |||
github.com/pkg/errors v0.9.1 | |||
github.com/pkg/term v0.0.0-20180730021639-bffc007b7fd5 // indirect | |||
github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 | |||
github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you needed to bump the version of this? Maybe as I review it will become clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not intentional, I do not know how to make mod tidy
not update it... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess that maybe one of the new dependencies needed a newer version of it then.
github.com/Azure/go-autorest/autorest v0.10.1 | ||
github.com/Azure/go-autorest/autorest/adal v0.8.3 | ||
github.com/Azure/go-autorest/autorest/azure/auth v0.4.2 | ||
github.com/DATA-DOG/go-sqlmock v1.4.1 | ||
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 | ||
github.com/apache/arrow/go/arrow v0.0.0-20191024131854-af6fa24be0db | ||
github.com/bonitoo-io/go-sql-bigquery v0.3.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you forked solcates
repository, but also merged some changes to there, does that mean we can use the original github.com/solcates/go-sql-bigquery
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I wasn't getting a response from the maintainer who seem to have abandoned the project, I gave up on hoping on the merge and continued with modifications in the bonitoo-io
fork. The owner then commented later that he _ had totally forgotten about this project_ and merged the PR, but the fork has diverted more from the original before that, into more pure SQL golang driver unlike the original which also contained some ORM stuff etc.
The original project does not seem to be active (no commits since Oct 27, 2019) and maintained (eg. PR was merge despite failing check due to some missing 3rd party lib).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks for this fine work.
This reverts commit 8d29781.
This PR add BigQuery support to
sql
package.bigquery
and uses URL format withbigquery
scheme as connection stringDone checklist