BigQuery backend #1170

tsdlovell · 2017-10-15T17:25:01Z

jreback · 2017-10-16T10:33:10Z

ibis/bigquery/tests/conftest.py

@@ -0,0 +1,80 @@
+import uuid
+import pytest
+


need a bq = pytest.importorskip('google.datalab.bigquery')

addressed in TST: use pytest.importorskip for bigquery dependency

jreback · 2017-10-16T10:34:37Z

ibis/__init__.py

@@ -40,6 +40,11 @@
    pass



need a small section in setup.py to add additional requires, something like: https://github.com/ibis-project/ibis/pull/1167/files#diff-2eeaed663bd0d25b7e608891384b7298

addressed in DOC: add bigquery_requires to setup.py

jreback · 2017-10-16T10:36:10Z

ibis/__init__.py

@@ -40,6 +40,11 @@
    pass

 try:
+    import ibis.bigquery.api as bigquery
+except ImportError:  # pip install ibis-framework[bigquery]
+    pass


need to add the pre-reqs to ci/requirements-2.7/3.6 (leave 3.4/4.5 so that they skip). need to mod appveyor.yml (see #1167 for what you need to add).

addressed in TST: add "git+https://github.com/googledatalab/..." as requirement

I don't think there's a pip package for pydatalab yet, so only have the github.com url

tsdlovell · 2017-10-16T11:34:08Z

How should the tests be getting bigquery credentials?

jreback · 2017-10-16T11:52:09Z

appveyor.yml

@@ -30,7 +30,7 @@ test_script:
  - "%CONDA% install conda=4.3.22 --channel conda-forge"
  - "%CONDA% create --name \"ibis_%PYTHON_VERSION%\" python=%PYTHON_VERSION% --channel conda-forge"
  - "%ACTIVATE% \"ibis_%PYTHON_VERSION%\""
-  - "pip install -e .\"[sqlite, postgres, visualization, pandas]\""
+  - "pip install -e .\"[sqlite, postgres, visualization, pandas, git+https://github.com/googledatalab/pydatalab@v1.1]\""


you should just be able to list this as: bigquery (pip should then call the appropriate requires)

jreback · 2017-10-16T17:40:37Z

How should the tests be getting bigquery credentials?

I guess we need a test account? @cpcloud

cpcloud · 2017-10-16T17:58:45Z

@tsdlovell You're failing the flake8 command:

./ibis/bigquery/api.py:1:1: F401 'ibis.bigquery.client.BigQueryClient' imported but unused
./ibis/bigquery/api.py:2:1: F401 'ibis.config.options' imported but unused
./ibis/bigquery/tests/conftest.py:7:1: E402 module level import not at top of file
./ibis/bigquery/tests/conftest.py:16:1: E303 too many blank lines (3)
Exited with code 1

cpcloud · 2017-10-16T18:04:40Z

ibis/bigquery/client.py

+
+    def __init__(self, project_id, dataset_id):
+        self.__project_id = project_id
+        self.__dataset_id = dataset_id


The __s seem unnecessary. Instance attributes starting with _ are considered private. Mutating them is out of the question and if someone decides to do that they are on their own. Besides, __ doesn't guarantee that you can't mutate these attributes, it just makes it extremely cumbersome.

cpcloud · 2017-10-16T18:07:01Z

ibis/bigquery/client.py

+
+
+@toolz.memoize
+def _bq_get_context(project_id):


Do we need both _bq_get_context and _bq_make_context?

making a context seems to be very costly in terms of time, so I expose a way to make one if you really want to but the default should probably be to just use get

cpcloud · 2017-10-16T18:08:53Z

ibis/bigquery/client.py

+        (table_id, dataset_id) = _ensure_split(name, dataset)
+        if dataset_id and dataset:
+            raise ValueError(
+                'Can\'t pass a fully qualified table name *AND* a dataset'


Use double quotes when embedding a single quote.

cpcloud · 2017-10-16T18:10:21Z

ibis/bigquery/client.py

+    'TIMESTAMP': dt.timestamp,
+    # 'BYTES': None,
+    # 'ARRAY': None,
+    # 'STRUCT': None,


Do you need any of these now that you have an implementation that can handle arrays and structs? What about BYTES?

cpcloud · 2017-10-16T18:11:37Z

ibis/bigquery/compiler.py

+    def extract_field_formatter(translator, expr):
+        op = expr.op()
+        arg = translator.translate(op.args[0])
+        return "extract({0!s} from {1!s})".format(sql_attr, arg)


The format argument indexes and !s are unnecessary.

I copied the format from the impala compiler. Can rewrite if desired.

No need, we can leave as is.

cpcloud · 2017-10-16T18:13:13Z

ibis/bigquery/tests/conftest.py

+
+
+@pytest.fixture(scope='session')
+def bq_context():


No need to prefix everything with _bq, we're already under bigquery/tests.

cpcloud · 2017-10-16T18:14:01Z

ibis/bigquery/tests/conftest.py

+def client(bq_dataset):
+    bq_name = bq_dataset.name
+    client = ibc.BigQueryClient(bq_name.project_id, bq_name.dataset_id)
+    ibis.options.default_backend = client


Don't set this for unit tests unless you are explicitly testing this, because it modifies global state.

cpcloud · 2017-10-16T18:15:20Z

ibis/bigquery/tests/test_client.py

+    assert str(result) == expected
+
+
+@pytest.mark.xfail()


xfail doesn't need parens

cpcloud · 2017-10-16T18:18:17Z

ibis/bigquery/client.py

+    if schema:
+        raise NotImplementedError()
+    else:
+        schema = dict()


You don't need this line.

cpcloud · 2017-10-16T18:21:41Z

ibis/bigquery/client.py

+    return bq.Table(dataset_id + '.' + table_id, _bq_get_context(project_id))
+
+
+def _bq_get_schema(table_id, dataset_id, project_id):


Why not keep a _context on the client (I see that you have a property based on the project_id) and use the bq.Table/bq.Dataset(s) objects directly instead of having these extra 1-2 line functions?

cpcloud · 2017-10-16T18:56:40Z

ibis/bigquery/compiler.py

+
+def _get_query(expr, context, params=None):
+    ast = build_ast(expr, context, params=params)
+    (query, *rest) = ast.queries


This isn't Python 2 compatible syntax.

tswast · 2017-10-16T22:14:20Z

ci/requirements-dev-3.6.yml

@@ -23,3 +23,4 @@ dependencies:
    - hdfs>=2.0.0
    - clickhouse-driver
    - clickhouse-sqlalchemy
+    - git+https://github.com/googledatalab/pydatalab@v1.1


I'm curious why you are using the Datalab libraries instead of google-cloud-python?

@tswast , its what I was exposed to in some demos of the tooling we can use to access gcp.

Is google-cloud-python the right thing to use?

Yeah, it's what we recommend at https://cloud.google.com/bigquery/docs/reference/libraries#client-libraries-install-python Datalab is great, but it's not really built to be used outside of the context of their notebook environment.

cpcloud · 2017-10-17T20:04:03Z

Disable your tests on Windows for now. Once we get everything passing on CircleCI then I'll add a Windows google-cloud-sdk installation to our appveyor CI. I can deal with adding testing on Windows once this is merged in.

cpcloud · 2017-10-17T20:07:59Z

ibis/bigquery/tests/conftest.py

+    return ibc.BigQueryClient(PROJECT_ID, DATASET_ID)
+
+
+@pytest.fixture(scope='session')


This needs to be yield_fixture.

For consistency or correctness?

https://docs.pytest.org/en/latest/yieldfixture.html

Since pytest-3.0, fixtures using the normal fixture decorator can use a yield statement to provide fixture values and execute teardown code, exactly like yield_fixture in previous versions.

Marking functions as yield_fixture is still supported, but deprecated and should not be used in new code.

Oh sweet! Thanks for the link.

cpcloud · 2017-10-17T20:09:23Z

ibis/bigquery/tests/conftest.py

+    assert client.project_id == bq_dataset.name.project_id
+    assert bq_dataset.name.project_id == bq_table._name_parts.project_id
+    assert client.dataset_id == bq_dataset.name.dataset_id
+    assert bq_dataset.name.dataset_id == bq_table._name_parts.dataset_id


Break these assertions out into a test. Assume the fixture is generated correctly if you want to use it expression/compilation/execution test or create two clients, one which you assume is constructed correctly and one whose properties you want to confirm are in fact correct.

cpcloud · 2017-10-17T20:10:21Z

ibis/bigquery/tests/conftest.py

+    assert bq_table.exists()
+    _df = bq_table.to_dataframe()
+    assert _df.equals(df)
+    yield client


This needs to be:

try: yield client finally: bq_table.delete() bq_dataset.delete()

otherwise you won't execute the delete calls when an exception is raised in the yielded-to function.

cpcloud · 2017-10-17T20:11:10Z

ibis/bigquery/tests/test_client.py

@@ -0,0 +1,97 @@
+import uuid


You shouldn't need this if you use ibis.util.guid().

cpcloud · 2017-10-17T20:11:40Z

ibis/bigquery/tests/test_client.py

+
+
+def test_array_execute(table, df):
+    col_name = df.select_dtypes(['float']).columns[0]


Why not just use the column name directly?

cpcloud · 2017-10-17T20:12:24Z

ibis/bigquery/tests/test_client.py

+
+
+def test_simple_aggregate_execute(table, df):
+    col_name = df.select_dtypes(['float']).columns[0]


Same as above, just hard code the column name.

cpcloud · 2017-10-17T20:13:26Z

ibis/bigquery/tests/test_client.py

+        client_with_table.dataset_id)
+    actual = client_with_table.list_tables()
+    expected = [el.name.table_id for el in bq_dataset.tables()]
+    assert sorted(actual) == sorted(expected)


I would hard code the exact names you expect to be there and assert that they are in fact there, otherwise this is just copying the implementation in a test.

I copied this from the postgres tests

assert db.list_tables() == self.con.list_tables()

I'll need to revisit that then :)

Hm, so what I'm actually referring to here is the use of the _proxy API, which is internal to ibis. You should do what the postgres tests do which is construct a database and then confirm that the database has the same tables as the client.

I think I'm doing the same thing here

def get_dataset(self, dataset_id): return bq.Dataset(dataset_id, self.context)

cpcloud · 2017-10-17T20:13:59Z

ibis/bigquery/tests/test_client.py

+    assert not t.exists()
+
+
+@pytest.mark.xfail()


No parens needed here.

tswast · 2017-10-18T19:24:39Z

ibis/bigquery/tests/test_client.py

+    assert bq_table.exists()
+
+    df_tuples = list(tuple(v.tolist()) for (k, v) in df.iterrows())
+    table_tuples = list(bq_table.fetch_data())


This test might be a bit flaky due to the use of table.insert_data(), which uses the streaming buffer. Rows in the streaming buffer don't appear in the list of rows from a table right away. (In fact, it could be as much as 90 minutes to get out of the streaming buffer.)

tswast · 2017-10-18T19:27:47Z

ibis/bigquery/tests/conftest.py

+    bq_table = bq_dataset.table(TABLE_ID, schema)
+    bq_table.create()
+    bq_table.reload()
+    bq_table.insert_data(tuples)


You'll have more reliable tests if you upload from a file and wait for the load job to complete. Example:

https://github.com/GoogleCloudPlatform/google-cloud-python/blob/6345c06ceb151d07761abb36f577909a3b76d7d4/docs/bigquery/snippets.py#L401-L408

Note: line-delimited JSON or Avro rather than CSV will be more robust in general, but CSV is probably fine for a test fixture like this.

cpcloud · 2017-10-18T22:31:26Z

ibis/bigquery/tests/conftest.py

+pytestmark = pytest.mark.bigquery
+
+
+PROJECT_ID = 'bq-for-ibis'


This should be 'ibis-gbq'.

cpcloud · 2017-10-18T22:45:47Z

ibis/bigquery/tests/conftest.py

+
+@pytest.fixture(scope='session')
+def client():
+    return ibc.BigQueryClient(PROJECT_ID, DATASET_ID)


Call ibis.bigquery.connect(PROJECT_ID, DATASET_ID) here.

tsdlovell · 2017-10-19T12:03:57Z

@jreback , @cpcloud : conda build tests are failing

Tests seem to be failing due bad dependency specification. What needs to be changed?

E   ImportError: No module named 'google'

cpcloud · 2017-10-19T13:30:27Z

ibis/bigquery/tests/test_client.py

+from ibis.bigquery.tests import conftest as conftest
+
+
+pytestmark = pytest.mark.bigquery


This is only for test files, not fixtures.

Sorry, I meant this for the conftest file.

cpcloud · 2017-10-19T13:31:42Z

ibis/bigquery/tests/test_client.py

+import ibis.util
+import ibis.expr.types as ir
+from ibis.bigquery import client as ibc
+from ibis.bigquery.tests import conftest as conftest


Don't import conftest here. Just make TABLE_ID a fixture and use it as a parameter to your other fixtures and tests.

cpcloud · 2017-10-19T13:33:01Z

ibis/bigquery/tests/conftest.py

+    json_file = six.BytesIO(
+        '\n'.join((v.to_json() for (k, v) in df.iterrows())).encode('UTF-8')
+    )
+    schema = infer_schema_from_df(df)


I uploaded functional_alltypes to the ibis-gbq project. Run your tests against that so we don't have to upload anything during tests. Then you don't need to import this function, and you'll be able to pass conda build CI.

cpcloud · 2017-10-19T13:35:06Z

ibis/bigquery/tests/conftest.py

+
+import pandas as pd
+import ibis.util
+import ibis.bigquery


Just import ibis. ibis.bigquery may not exist if someone doesn't have google-cloud-bigquery installed. This module should be importable without the required dependencies so that pytest can still run.

cpcloud · 2017-10-19T13:37:18Z

ibis/bigquery/tests/test_client.py

+@pytest.mark.xfail
+def test_df_upload(client):
+    expected = pd.DataFrame(dict(a=[1], b=[2.], c=['a'], d=[True]))
+    schema = ibc.infer_schema_from_df(expected)


This should be done inside the insert method on the table (you may need to define it if it doesn't exist), so that users are not exposed this.

I would leave this functionality for another PR so that you don't have to deal with this stuff in this one.

cpcloud · 2017-10-19T13:38:45Z

ibis/bigquery/tests/test_client.py

+    t = client.table('rando', schema)
+    t.upload(expected)
+    result = t.execute()
+    t.delete()


DELETE means something different than DROP, but it looks like you're dropping the table here. If that's the case then you should implement this via the drop method so the API is consistent with the rest of ibis.

Same as insert, I would leave this for the next PR and remove it in this one.

cpcloud · 2017-10-19T13:39:33Z

ibis/bigquery/tests/test_client.py

+    expected = pd.DataFrame(dict(a=[1], b=[2.], c=['a'], d=[True]))
+    schema = ibc.infer_schema_from_df(expected)
+    t = client.table('rando', schema)
+    t.upload(expected)


Similar to my other comments in this test, this functionality should be exposed through the insert method.

cpcloud · 2017-10-22T16:13:44Z

Merging.

cpcloud · 2017-10-22T16:15:33Z

Thanks @tsdlovell, great to get this going!

wesm · 2017-10-23T01:54:22Z

Huzzah! Thanks @tsdlovell!

jreback added the feature Features or general enhancements label Oct 16, 2017

jreback reviewed Oct 16, 2017

View reviewed changes

tsdlovell force-pushed the bigquery-backend branch from db3caf9 to 65ae6d8 Compare October 16, 2017 11:30

jreback reviewed Oct 16, 2017

View reviewed changes

tsdlovell force-pushed the bigquery-backend branch from bcd29b5 to 29e70da Compare October 16, 2017 17:38

cpcloud requested changes Oct 16, 2017

View reviewed changes

cpcloud self-assigned this Oct 16, 2017

cpcloud reviewed Oct 16, 2017

View reviewed changes

tsdlovell force-pushed the bigquery-backend branch 2 times, most recently from a799d6a to 903951e Compare October 16, 2017 20:31

tswast reviewed Oct 16, 2017

View reviewed changes

cpcloud added the bigquery label Oct 17, 2017

cpcloud added this to the 0.13 milestone Oct 17, 2017

cpcloud requested changes Oct 17, 2017

View reviewed changes

tswast reviewed Oct 18, 2017

View reviewed changes

tsdlovell force-pushed the bigquery-backend branch from 39930db to b194615 Compare October 18, 2017 21:19

cpcloud reviewed Oct 18, 2017

View reviewed changes

ibis/bigquery/tests/conftest.py Outdated

pytestmark = pytest.mark.bigquery

PROJECT_ID = 'bq-for-ibis'

Copy link

Member

cpcloud Oct 18, 2017

Choose a reason for hiding this comment

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

This should be 'ibis-gbq'.

cpcloud reviewed Oct 18, 2017

View reviewed changes

cpcloud reviewed Oct 19, 2017

View reviewed changes

dlovell and others added 21 commits October 22, 2017 10:37

TST: use correct PROJECT_ID

f3cd0b5

BUG: ibis.bigquery.api: add connect function

bb66108

TST: refactor

6a6f515

TST: use functional_alltypes

73c553c

TST

eaef1d1

PEP8

18d1699

CLN: remove uncessary noqa

1d816f3

REF

831ed7d

ENH: add _BigQueryAPIProxy.run_statement

8a2e7a6

Add struct fixture and field access test

85f877e

Add array expr tests

7370908

Implement struct/array op translation rules

e2bd298

Reuse SQLClient functionality in bigquery

6e5c1ff

BUG: _execute: pass proxy's client

889b1bb

TST: account for use of BigQueryCursor

326812b

CLN: remove comments

b1a5b52

DOC: connect: add docstring

e22cba0

REF: make method names/args match ibis.client.Client

772f26d

DOC: s/Impala/BigQuery

9c41702

REF: better naming

9ad8f08

TST: add test of BigQueryClient.database method

0160276

tsdlovell force-pushed the bigquery-backend branch from 06ba847 to 0160276 Compare October 22, 2017 14:37

cpcloud modified the milestones: 0.13, 0.12 Oct 22, 2017

cpcloud closed this in 120738e Oct 22, 2017

cpcloud changed the title ~~WIP: bigquery backend~~ BigQuery backend Oct 22, 2017

tsdlovell deleted the bigquery-backend branch October 22, 2017 16:35

		return bq.Table(dataset_id + '.' + table_id, _bq_get_context(project_id))


		def _bq_get_schema(table_id, dataset_id, project_id):

		return ibc.BigQueryClient(PROJECT_ID, DATASET_ID)


		@pytest.fixture(scope='session')



		def test_array_execute(table, df):
		col_name = df.select_dtypes(['float']).columns[0]



		def test_simple_aggregate_execute(table, df):
		col_name = df.select_dtypes(['float']).columns[0]

		from ibis.bigquery.tests import conftest as conftest


		pytestmark = pytest.mark.bigquery

BigQuery backend #1170

BigQuery backend #1170

Conversation

tsdlovell commented Oct 15, 2017 • edited by jreback 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

tsdlovell commented Oct 16, 2017

Choose a reason for hiding this comment

jreback commented Oct 16, 2017

cpcloud commented Oct 16, 2017

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

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

tsdlovell Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud commented Oct 17, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsdlovell commented Oct 19, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud commented Oct 22, 2017

cpcloud commented Oct 22, 2017

wesm commented Oct 23, 2017

tsdlovell commented Oct 15, 2017 •

edited by jreback

Loading

tsdlovell Oct 16, 2017 •

edited

Loading

cpcloud commented Oct 17, 2017 •

edited

Loading