-
Notifications
You must be signed in to change notification settings - Fork 99
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(go/adbc/driver): add support for Google BigQuery #1722
feat(go/adbc/driver): add support for Google BigQuery #1722
Conversation
525d7c2
to
568d677
Compare
@zeroshade do you think you could give this a brief scan and make sure things are on the right track? |
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.
This is a great start!! Thanks!
Left a ton of comments for you
Hi @zeroshade, thank you so much for the code review! And sorry that I only picked up some Go skills in the past week based on the snowflake implementation. The issues you mentioned should've been fixed, and I'll implement the rest APIs and try to stick to these standards in Go :) |
Hi I've updated and implemented a bit more. Although I'm not 100% sure if this is the right/best way to do some functions... I'll be happy to make any changes. Besides that, I also updated the todo list in the top comment. While I'd like to implement these functions as much as I can, please do let me know if we can put off any of them and address them in another PR. :) |
all those TODOs are fine to split into later PRs |
Got it! Then we probably can merge this first once we're happy about with it. I'll do separate PRs for the left bits. :) And once again, thank you all for the great help and your time for the code review. @lidavidm @zeroshade ❤️ |
I agree with @lidavidm that the TODOs are fine to split into later PRs. Thanks for your work here! I'll give this a new review pass tomorrow. For now I approved the CI to run, looks like there's some pre-commit formatting/linting issues you have to resolve among other failures. |
Thank you very much @zeroshade!! I'll resolve these issues along with any issues you may point out in the code review 😃 |
841d2d7
to
afd1e77
Compare
afd1e77
to
72f998a
Compare
I think you'll want to try that rebase again 😅 |
72f998a
to
e68ed26
Compare
git is hard... now it should work I guess |
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 did a first pass reviewing this. I'll do another pass tomorrow to get the rest
As pointed out by @zeroshade [here](#1722 (comment)), we should fix the formatting of the comment.
tl;dr: yes. According to the replies in googleapis/google-cloud-go#10044 and docs for BigQuery, the answer is yes, we have to enumerate projects and datasets (and as said in the reply, to enumerate projects we have to use ResourceManager, as current implementation of bigquery is not designed or possible to achieve this); or we're effectively limited in a single region of a project (it's impossible to query all datasets that stored in multiple regions using a single query). Otherwise we would have to wait for Google to implement this feature, using a single SQL query to get all datasets in a project regardless of their location. |
As pointed out by @zeroshade [here](#1722 (comment)), this should be handled by doing `adbc.Error{Msg: ctx.Err(), Code:....}`.
Hi @zeroshade, sorry for the ping! I've done writing basic tests for BigQuery, and I fully understand that you may be quite busy recently, but I was wondering if you perhaps have any free moment for a review or leave me some todo points so that this can get merged or push forward? I'll be very happy to share my BigQuery credentials for you to run the tests. Please feel free to email/dm me. My email address is the same as the one used in the commits, and you can also find me on the Gopher channel. ;) |
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.
This is my last set of nitpicks that I can see. I'm happy with the PR as-is after these changes, @joellubi any further thoughts from you on this? Given my recent busy schedule, i'm gonna rely on Joel to help you get this merged!
thanks for all this work, looking forward to getting this in. sorry it's taking so long
@zeroshade Many thanks for the code review! I've fixed all of them except the last one -- the chan will close early after my refactor and I didn't quite figure out a way do avoid that so I left it as is. Sorry I'm not quite sure how to properly do this in Go, so if anyone has any suggestions or hints on this, I'll be happy to make further changes!
And no worries! It was all very interesting for me and I've learnt a lot about Go programming from doing it and from these code reviews! |
I've updated to use |
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.
Just a few comments, looks good to me overall. I don't have a BigQuery environment at the moment to execute the test suite in so I haven't verified the tests myself. I pointed out a few places where I suspect some issues may come up when the tests are run, but any inconsistencies there can be fixed once the tests are added to CI and become load-bearing.
Many thanks for the code review @joellubi! Sorry I didn't notice some of these test flags, and they should be corrected now. If there is no other apparent issues, I guess perhaps we can merge this PR for now? I plan to make separate PRs for bulk ingestion and other todo items in this PR. ;) |
Just those last two nitpicks from me, I'm good to merge this after they are addressed. The remaining stuff can be handled in a follow-up |
39be2ff
to
32c9200
Compare
This PR should be ready to be merged 🎉~ I'll make a checklist for these todos in the code today or tomorrow. And massive thanks for your time reviewing my code and these valuable feedback! |
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.
Thanks @cocoa-xu!
It looks like we're good to merge? |
Yes, it should be ready! And one left thing for the repo maintainers is, it would need a test account, generate and save JSON credentials in the repo secrets when we want to do any integration tests. :) |
Thank you @cocoa-xu for the huge contribution! |
Quick follow up to #1722 meson test skips everything at the moment - didn't see this getting tested in CI so assuming its something done offline
Hi this PR is a preliminary Go implementation for Google BigQuery as the preferred approach to PR #1717.
Currently it supports query functionality as a proof of concept, users can
It gives the same results as in #1717 using this driver in Elixir using elixir-explorer/adbc.
There're still a few thing to be done:
implement GetInfo, GetTableSchema and other functions for BigQuery's AdbcConnection and AdbcStatementget table constraints and return them in corresponding info objects(currently impossible to do so)Bind
andBindStream
ExecuteSchema
?ReadPartition
andExecutePartitions
?