Skip to content
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

Add support for Google BigQuery #7

Merged
merged 11 commits into from
May 27, 2022

Conversation

aleDsz
Copy link
Member

@aleDsz aleDsz commented May 20, 2022

@aleDsz aleDsz added the enhancement New feature or request label May 20, 2022
@aleDsz aleDsz self-assigned this May 20, 2022
@aleDsz aleDsz marked this pull request as draft May 20, 2022 18:11
@aleDsz aleDsz force-pushed the feature/support-bigquery branch 2 times, most recently from 8f50814 to 26bd5b2 Compare May 24, 2022 16:36
@aleDsz aleDsz force-pushed the feature/support-bigquery branch from 26bd5b2 to 2eeb12d Compare May 25, 2022 13:46
@aleDsz aleDsz force-pushed the feature/support-bigquery branch from 88bba71 to 0b36c10 Compare May 26, 2022 13:54
lib/kino_db/connection_cell.ex Outdated Show resolved Hide resolved
lib/kino_db/sql_cell.ex Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
test/kino_db/connection_cell_test.exs Outdated Show resolved Hide resolved
test/kino_db/connection_cell_test.exs Outdated Show resolved Hide resolved
test/kino_db/connection_cell_test.exs Outdated Show resolved Hide resolved
@wojtekmach
Copy link

What happens if we get an error from google? e.g. there's a syntax error in the query which would probably result in a HTTP 400. I believe in that case ReqBigQuery would return a %Req.Response{status: 400, ...} but KinoDB cannot do anything with it. Should it raise a good error message?

Two things:

  1. If we're missing an integration test for such invalid query in req_bigquery, please add one.
  2. If we want a reasonable error message out of Req.Response, we can change Req.new to Req.new(http_errors: :raise).

@aleDsz aleDsz force-pushed the feature/support-bigquery branch from f000183 to f156057 Compare May 26, 2022 19:36
@aleDsz
Copy link
Member Author

aleDsz commented May 26, 2022

I'll implement how to show the Google BigQuery help box with Service Account credentials JSON generation, so this PR isn't ready to merge yet.

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐱 !!

lib/assets/connection_cell/main.js Show resolved Hide resolved
lib/assets/connection_cell/main.js Outdated Show resolved Hide resolved
lib/assets/connection_cell/main.js Outdated Show resolved Hide resolved
test/kino_db/connection_cell_test.exs Outdated Show resolved Hide resolved
lib/kino_db/sql_cell.ex Outdated Show resolved Hide resolved
lib/kino_db/sql_cell.ex Outdated Show resolved Hide resolved
lib/kino_db.ex Outdated Show resolved Hide resolved
lib/kino_db.ex Outdated Show resolved Hide resolved
lib/kino_db.ex Outdated Show resolved Hide resolved
lib/kino_db/connection_cell.ex Outdated Show resolved Hide resolved
lib/kino_db.ex Outdated Show resolved Hide resolved
@aleDsz aleDsz force-pushed the feature/support-bigquery branch from f156057 to 5ba1094 Compare May 27, 2022 14:52
@aleDsz
Copy link
Member Author

aleDsz commented May 27, 2022

So the help box for Google BigQuery looks like this:

Screenshot_20220527_115410

@aleDsz aleDsz marked this pull request as ready for review May 27, 2022 14:55
@aleDsz aleDsz requested a review from jonatanklosko May 27, 2022 16:36
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

@aleDsz aleDsz merged commit 9defa08 into livebook-dev:main May 27, 2022
@wojtekmach
Copy link

@aleDsz I believe we need to do one more change. If the user didn't select a credentials file, we should start Goth without the :source option so it can do the application default credential thing and if that fails, there is a good error message. Only if the user dropped credentials file, we'd explicitly set :source. WDYT?

@aleDsz
Copy link
Member Author

aleDsz commented Jun 28, 2022

@aleDsz I believe we need to do one more change. If the user didn't select a credentials file, we should start Goth without the :source option so it can do the application default credential thing and if that fails, there is a good error message. Only if the user dropped credentials file, we'd explicitly set :source. WDYT?

I agree, we should remove :source so the metadata or system env could be used by default 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants