BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic. #9170

shubha-rajan · 2019-09-04T02:18:18Z

Third of 3 PRs towards resolving #9105 as described in review for #9147
Screenshot of feature in notebook:

added default patch to unit tests

crwilcox · 2019-09-10T19:07:08Z

bigquery/google/cloud/bigquery/magics.py

+            rows = client.list_rows(table_id, max_results=max_results)
+        except Exception as ex:
+            error = str(ex)
+        if error:


could this be simplified to return from the exception?

it definitely can. fixed in 7a13ca4

crwilcox · 2019-09-10T19:14:16Z

bigquery/google/cloud/bigquery/magics.py

@@ -434,6 +444,26 @@ def _cell_magic(line, query):
    else:
        max_results = None

+    error = None
+
+    if not re.search(r"\s", query.rstrip()):


What scenario is this protecting against? Is there a scenario where unicode strings will not remove space characters via rstrip?

What do we expect in the else case?

Related: Does this repository enforce code coverage? Is there a case where we test there being unstoppable whitespace and not taking this branch?

This condition is actually testing for queries that contain whitespace characters that aren't removed by rstrip (so any whitespace that isn't a trailing newline). The assumption being made (as described in #9105 ) is that anything containing whitespace is a SQL query and won't take this branch, while anything string without whitespace is a table_id and will take this branch.

So anything regular SQL query would fall into the else case. There are tests that check whether query strings without spaces will be interpreted as table IDs and a test that checks if a string without whitespace that isn't a valid table_id raises an appropriate error message.

I can add a comment in the code if that would be helpful.

I would also write this down in a comment, i.e. that anything without whitespace is assumed to be table identifier which triggers a different use case of the magic command.

added comment in d663104

plamut

The change generally works.

I did notice, however, that the logic is sensitive to the leading whitespace:

┌───────────────────────────────────────────┐
│ %%bigquery --max_results=6                │                 
|  bigquery-public-data.samples.shakespeare │
└───────────────────────────────────────────┘
  Executing query with job ID: a0f195e6-f748-4c42-8c3b-d89abaa37c9f
  Query executing: 0.92s

  ERROR:
    400 Syntax error: Unexpected identifier "bigquery" at [1:3]

Wouldn't it be better to strip the whitespace on both sides first, or why we only do .rstrip()?

plamut · 2019-09-20T22:55:10Z

bigquery/google/cloud/bigquery/magics.py

    try:
        query_job = _run_query(client, query, job_config=job_config)
    except Exception as ex:
-        error = str(ex)
+        return _print_error(str(ex), args.destination_var)


This can be slightly confusing for the reader at first glance, because _print_error() itself does not return anything, it just has a side effect. I would simply express the same in two lines (a sole return in its own).

fixed in 6f2dd64

plamut · 2019-09-20T22:58:52Z

bigquery/google/cloud/bigquery/magics.py

@@ -434,6 +444,26 @@ def _cell_magic(line, query):
    else:
        max_results = None

+    error = None
+
+    if not re.search(r"\s", query.rstrip()):


I would also write this down in a comment, i.e. that anything without whitespace is assumed to be table identifier which triggers a different use case of the magic command.

plamut · 2019-09-20T23:28:37Z

bigquery/google/cloud/bigquery/magics.py

@@ -434,6 +444,21 @@ def _cell_magic(line, query):
    else:
        max_results = None

+    if not re.search(r"\s", query.rstrip()):
+        table_id = query.rstrip()


BTW, stripping whitespace from the same value again is not necessary, we could store the stripped string into a variable the first time we do it.

fixed in d663104

plamut

LGTM, thanks!

It might cause a merge conflict with #9245, but I'll address that there.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 4, 2019

cell magic accepts table_ids instead of queries

77e62c5

added default patch to unit tests

shubha-rajan force-pushed the bq-table-id-instead-of-query branch from 6761217 to 77e62c5 Compare September 6, 2019 05:33

shubha-rajan marked this pull request as ready for review September 6, 2019 14:49

shubha-rajan requested a review from a team September 6, 2019 14:49

crwilcox reviewed Sep 10, 2019

View reviewed changes

simplified error handling to return from exception

7a13ca4

shubha-rajan requested a review from crwilcox September 10, 2019 22:50

plamut added the api: bigquery Issues related to the BigQuery API. label Sep 19, 2019

plamut reviewed Sep 20, 2019

View reviewed changes

shubha-rajan added 3 commits September 20, 2019 17:36

added comment and updated to use strip instead of rstrip

d663104

blacken/lint

abcd29c

reformatted return statement

6f2dd64

shubha-rajan requested a review from plamut September 21, 2019 00:46

removed trailing whitespace

428368f

plamut approved these changes Sep 23, 2019

View reviewed changes

plamut merged commit ee0f70a into googleapis:master Sep 23, 2019

plamut mentioned this pull request Sep 25, 2019

BigQuery: preview / list_rows command for %%bigquery Jupyter magic #9105

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic. #9170

BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic. #9170

shubha-rajan commented Sep 4, 2019 •

edited

Loading

crwilcox Sep 10, 2019

shubha-rajan Sep 10, 2019

crwilcox Sep 10, 2019

crwilcox Sep 10, 2019

shubha-rajan Sep 10, 2019

shubha-rajan Sep 10, 2019

plamut Sep 20, 2019

shubha-rajan Sep 21, 2019

plamut left a comment •

edited

Loading

plamut Sep 20, 2019

shubha-rajan Sep 21, 2019 •

edited

Loading

plamut Sep 20, 2019

plamut Sep 20, 2019

shubha-rajan Sep 21, 2019

plamut left a comment

BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic. #9170

BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic. #9170

Conversation

shubha-rajan commented Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plamut left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha-rajan Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plamut left a comment

Choose a reason for hiding this comment

shubha-rajan commented Sep 4, 2019 •

edited

Loading

plamut left a comment •

edited

Loading

shubha-rajan Sep 21, 2019 •

edited

Loading