Added BigQuery client method (create table) for Tables API #3

lbristol88 · 2019-03-29T18:44:54Z

No description provided.

… bqml

tswast · 2019-03-29T22:03:48Z

@engelke please review

We're working from my fork, as this is dependent on googleapis#7562 which adds the samples directory.

tswast · 2019-03-29T22:04:50Z

bigquery/samples/create_table.py

+# limitations under the License.
+
+
+def create_table():


I'd expect to see a client argument here (and also the table ID).

Do the tests pass when you run pytest on the samples directory?

Yes! Tests have now been updated to pass. 😄

tswast · 2019-03-29T22:07:29Z

bigquery/samples/tests/conftest.py

+        now.strftime("%Y%m%d%H%M%S"), uuid.uuid4().hex[:8]
+    )
+    table = client.create_table("{}.{}".format(dataset_id, table_id))
+    yield "{}".format(table.full_table_id)


Don't use the full_table_id property for this. Use table.project, table.dataset_id, and table.table_id.

The full_table_id property is in legacy SQL format, and also I don't really trust that it's always populated.

Updated with your suggestion

tswast · 2019-03-29T22:09:57Z

bigquery/samples/tests/conftest.py

+    table_id = "example_table_{}_{}".format(
+        now.strftime("%Y%m%d%H%M%S"), uuid.uuid4().hex[:8]
+    )
+    table = client.create_table("{}.{}".format(dataset_id, table_id))


Since you're creating a table in your sample, you don't actually want to create one in the test fixture.

We might want to rename this to random_table_id to indicate that no table is actually created.

I renamed the fixture to random_table_id and removed the creation of the table in the fixture.

tswast · 2019-03-29T22:12:03Z

bigquery/samples/tests/test_table_samples.py

+    create_table.create_table(client, project_id, dataset_id, table_id)
+    out, err = capsys.readouterr()
+    assert create_table in out
+    assert table.table_id == "my_table"


Where's the table object coming from? Potentially you could return it from the create_table function, but in this case I think we can get a good enough of a sense that what we wanted to happen actually happened from out.

Updated the test to work as you suggested.

tswast · 2019-03-29T22:12:28Z

bigquery/samples/tests/test_table_samples.py

+from .. import create_table
+
+
+def test_table_samples(capsys, client, project_id, dataset_id, table_id):


Rename table_id to random_table_id when you rename the fixture.

I updated this as suggested.

tswast · 2019-03-29T22:13:31Z

bigquery/samples/tests/test_table_samples.py

+    """
+    create_table.create_table(client, project_id, dataset_id, table_id)
+    out, err = capsys.readouterr()
+    assert create_table in out


Assert that some string you expect appears in out, not the sample module. In this case, looking for table_id in out would give a good indication that the table was created successfully.

I have updated this as you suggested.

tswast

Much improved, thank you! A few more items to cleanup in the tests.

tswast · 2019-04-02T16:18:52Z

bigquery/samples/tests/conftest.py

-    table = client.create_table("{}.{}".format(dataset_id, table_id))
-    yield "{}".format(table.full_table_id)
-    client.delete_table(table, delete_contents=True)
+    yield "{}.{}".format(dataset_id, random_table_id)


Nit, since you don't have to do any cleanup, this can now be return instead of yield.

Replaced both table and dataset fixtures to return instead of yield.

tswast · 2019-04-02T16:19:46Z

bigquery/samples/tests/test_table_samples.py

-def test_table_samples(capsys, client, project_id, dataset_id, table_id):
-    """Since creating a table is a long operation, test all table model samples
+def test_table_samples(capsys, client, random_table_id):
+    """Since creating a table is a long operation, test all table samples


Creating a table isn't all that long of an operation. You can test each sample separately. This docstring comment can be removed.

Removed this docstring.

tswast · 2019-04-02T16:20:14Z

bigquery/samples/tests/test_table_samples.py

@@ -15,11 +15,10 @@
 from .. import create_table


-def test_table_samples(capsys, client, project_id, dataset_id, table_id):
-    """Since creating a table is a long operation, test all table model samples
+def test_table_samples(capsys, client, random_table_id):


This function can be renamed to test_create_table.

Changed the name as directed.

tswast · 2019-04-02T16:21:45Z

bigquery/samples/tests/conftest.py

@@ -25,6 +25,20 @@ def client():
    return bigquery.Client()


+@pytest.fixture
+def project_id(client):


Since you don't use the project_id fixture anywhere, this can be removed.

tswast · 2019-04-02T16:37:11Z

Oh, and one last thing. Let's remove the bigquery_create_table snippet from docs/snippets.py too.

lbristol88 · 2019-04-02T20:29:21Z

The test_create_table_sample snippet is now removed from the bigquery collection.

tswast · 2019-04-02T20:40:48Z

bigquery/samples/tests/conftest.py

 @pytest.fixture
 def dataset_id(client):
    now = datetime.datetime.now()
    dataset_id = "python_samples_{}_{}".format(
        now.strftime("%Y%m%d%H%M%S"), uuid.uuid4().hex[:8]
    )
    dataset = client.create_dataset(dataset_id)
-    yield "{}.{}".format(dataset.project, dataset.dataset_id)
+    return "{}.{}".format(dataset.project, dataset.dataset_id)


Sorry, this should stay yield, because it needs to clean up the dataset after the test runs.

tswast

🎉

engelke · 2019-04-02T22:24:44Z

LGTM

* iam proposal #3 maintain compatibility with defaultdict remove in place raise KeyError on delete update deprecation for dict-key access and factory methods clean up maintain compatibility - removing duplicate in __setitems__ check for conditions for dict access remove empty binding fix test accessing private var _bindings fix(tests): change version to make existing tests pass tests: add tests for getitem, delitem, setitem on v3 and conditions test policy.bindings property fixlint black sort bindings by role when converting to api repr add deprecation warning for iam factory methods update deprecation message for role methods make Policy#bindings.members a set update policy docs fix docs make docs better fix: Bigtable policy class to use Policy.bindings add from_pb with conditions test add to_pb condition test blacken fix policy __delitem__ add docs on dict access do not modify binding in to_apr_repr * feat(storage): support requested_policy_version for get_iam_policy * add system-test * add ref doc sample to get_iam_policy * add requested_policy_version to blob * fix tests * nit: typo * blacken * fix docs build * format docs * remove unused variables

…ogleapis#10001) * iam proposal #3 maintain compatibility with defaultdict remove in place raise KeyError on delete update deprecation for dict-key access and factory methods clean up maintain compatibility - removing duplicate in __setitems__ check for conditions for dict access remove empty binding fix test accessing private var _bindings fix(tests): change version to make existing tests pass tests: add tests for getitem, delitem, setitem on v3 and conditions test policy.bindings property fixlint black sort bindings by role when converting to api repr add deprecation warning for iam factory methods update deprecation message for role methods make Policy#bindings.members a set update policy docs fix docs make docs better fix: Bigtable policy class to use Policy.bindings add from_pb with conditions test add to_pb condition test blacken fix policy __delitem__ add docs on dict access do not modify binding in to_apr_repr * feat(bigtable): support requested_policy_version to instance * fix passing requested_policy_version to pb2 * add unit test * add unit test

lbristol88 added 2 commits March 29, 2019 11:14

Added client method (create table) for Tables API

2616602

Merge branch 'bqml' of github.com:lbristol88/google-cloud-python into…

7970536

… bqml

tswast requested changes Mar 29, 2019

View reviewed changes

lbristol88 added 3 commits April 1, 2019 21:21

Updated create table to address review feedback.

b3f61b9

Updated to fix typo in test docstring.

eed701f

Removed unneeded parameter.

19124c0

tswast requested changes Apr 2, 2019

View reviewed changes

lbristol88 added 2 commits April 2, 2019 13:19

Removed test_create_table from snippets collection

7b35b52

Removed unnecessary fixture, docstring, and made changes as directed.

64592ba

tswast changed the base branch from bqml to bq-snippets April 2, 2019 20:39

tswast reviewed Apr 2, 2019

View reviewed changes

Updated dataset fixture to yield instead of return.

cd66b9a

tswast approved these changes Apr 2, 2019

View reviewed changes

tswast merged commit 2972c61 into tswast:bq-snippets Apr 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added BigQuery client method (create table) for Tables API #3

Added BigQuery client method (create table) for Tables API #3

lbristol88 commented Mar 29, 2019

tswast commented Mar 29, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast Mar 29, 2019

lbristol88 Apr 2, 2019

tswast left a comment

tswast Apr 2, 2019

lbristol88 Apr 2, 2019

tswast Apr 2, 2019

lbristol88 Apr 2, 2019

tswast Apr 2, 2019

lbristol88 Apr 2, 2019

tswast Apr 2, 2019

lbristol88 Apr 2, 2019

tswast commented Apr 2, 2019

lbristol88 commented Apr 2, 2019 •

edited

Loading

tswast Apr 2, 2019

lbristol88 Apr 2, 2019

tswast left a comment

engelke commented Apr 2, 2019

		from .. import create_table


		def test_table_samples(capsys, client, project_id, dataset_id, table_id):

Added BigQuery client method (create table) for Tables API #3

Added BigQuery client method (create table) for Tables API #3

Conversation

lbristol88 commented Mar 29, 2019

tswast commented Mar 29, 2019

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

tswast left a 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

tswast commented Apr 2, 2019

lbristol88 commented Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

engelke commented Apr 2, 2019

lbristol88 commented Apr 2, 2019 •

edited

Loading