From 7f4bc54c5c9dd52e2c23154ddf6db5c772483ac8 Mon Sep 17 00:00:00 2001 From: Tim Swast Date: Wed, 8 May 2019 13:04:21 -0700 Subject: [PATCH 1/4] Prevent error when time partitioning is populated with empty dict As reported in internal bug 131167013, calling `table.time_partitioning` can sometimes fail with `KeyError: 'type'` when attempting to read this property on a non-partitioned table. --- bigquery/google/cloud/bigquery/table.py | 4 +-- bigquery/tests/unit/test_table.py | 48 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/bigquery/google/cloud/bigquery/table.py b/bigquery/google/cloud/bigquery/table.py index 46213d5fe8bf..864fff4458b1 100644 --- a/bigquery/google/cloud/bigquery/table.py +++ b/bigquery/google/cloud/bigquery/table.py @@ -1787,7 +1787,7 @@ def type_(self): """google.cloud.bigquery.table.TimePartitioningType: The type of time partitioning to use. """ - return self._properties["type"] + return self._properties.get("type") @type_.setter def type_(self, value): @@ -1849,7 +1849,7 @@ def from_api_repr(cls, api_repr): google.cloud.bigquery.table.TimePartitioning: The ``TimePartitioning`` object. """ - instance = cls(api_repr["type"]) + instance = cls() instance._properties = api_repr return instance diff --git a/bigquery/tests/unit/test_table.py b/bigquery/tests/unit/test_table.py index 18ca125e804c..5705708269b4 100644 --- a/bigquery/tests/unit/test_table.py +++ b/bigquery/tests/unit/test_table.py @@ -871,6 +871,54 @@ def test__build_resource_w_custom_field_not_in__properties(self): with self.assertRaises(ValueError): table._build_resource(["bad"]) + def test_time_partitioning_getter(self): + from google.cloud.bigquery.table import TimePartitioning + from google.cloud.bigquery.table import TimePartitioningType + + dataset = DatasetReference(self.PROJECT, self.DS_ID) + table_ref = dataset.table(self.TABLE_NAME) + table = self._make_one(table_ref) + + table._properties["timePartitioning"] = { + "type": "DAY", + "field": "col1", + "expirationMs": "123456", + "requirePartitionFilter": False, + } + self.assertIsInstance(table.time_partitioning, TimePartitioning) + self.assertEqual(table.time_partitioning.type_, TimePartitioningType.DAY) + self.assertEqual(table.time_partitioning.field, "col1") + self.assertEqual(table.time_partitioning.expiration_ms, 123456) + self.assertFalse(table.time_partitioning.require_partition_filter) + + def test_time_partitioning_getter_w_none(self): + dataset = DatasetReference(self.PROJECT, self.DS_ID) + table_ref = dataset.table(self.TABLE_NAME) + table = self._make_one(table_ref) + + table._properties["timePartitioning"] = None + self.assertIsNone(table.time_partitioning) + + del table._properties["timePartitioning"] + self.assertIsNone(table.time_partitioning) + + def test_time_partitioning_getter_w_empty(self): + from google.cloud.bigquery.table import TimePartitioning + + dataset = DatasetReference(self.PROJECT, self.DS_ID) + table_ref = dataset.table(self.TABLE_NAME) + table = self._make_one(table_ref) + + # Even though there are required properties according to the API + # specification, sometimes time partitioning is populated as an empty + # object. See internal bug 131167013. + table._properties["timePartitioning"] = {} + self.assertIsInstance(table.time_partitioning, TimePartitioning) + self.assertIsNone(table.time_partitioning.type_) + self.assertIsNone(table.time_partitioning.field) + self.assertIsNone(table.time_partitioning.expiration_ms) + self.assertIsNone(table.time_partitioning.require_partition_filter) + def test_time_partitioning_setter(self): from google.cloud.bigquery.table import TimePartitioning from google.cloud.bigquery.table import TimePartitioningType From 14a04951d2a1ab12fb2f02ca72487b1fbc68ff6a Mon Sep 17 00:00:00 2001 From: Tim Swast Date: Wed, 8 May 2019 13:40:16 -0700 Subject: [PATCH 2/4] Add tests for TimePartitioning.from_api_repr --- bigquery/tests/unit/test_table.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/bigquery/tests/unit/test_table.py b/bigquery/tests/unit/test_table.py index 5705708269b4..c8acff3a076a 100644 --- a/bigquery/tests/unit/test_table.py +++ b/bigquery/tests/unit/test_table.py @@ -2259,6 +2259,22 @@ def test_constructor_explicit(self): self.assertEqual(time_partitioning.expiration_ms, 10000) self.assertTrue(time_partitioning.require_partition_filter) + def test_from_api_repr_empty(self): + from google.cloud.bigquery.table import TimePartitioningType + + klass = self._get_target_class() + + # Even though there are required properties according to the API + # specification, sometimes time partitioning is populated as an empty + # object. See internal bug 131167013. + api_repr = {} + time_partitioning = klass.from_api_repr(api_repr) + + self.assertIsNone(time_partitioning.type_) + self.assertIsNone(time_partitioning.field) + self.assertIsNone(time_partitioning.expiration_ms) + self.assertIsNone(time_partitioning.require_partition_filter) + def test_from_api_repr_minimal(self): from google.cloud.bigquery.table import TimePartitioningType @@ -2271,6 +2287,14 @@ def test_from_api_repr_minimal(self): self.assertIsNone(time_partitioning.expiration_ms) self.assertIsNone(time_partitioning.require_partition_filter) + def test_from_api_repr_doesnt_override_type(self): + from google.cloud.bigquery.table import TimePartitioningType + + klass = self._get_target_class() + api_repr = {"type": "HOUR"} + time_partitioning = klass.from_api_repr(api_repr) + self.assertEqual(time_partitioning.type_, "HOUR") + def test_from_api_repr_explicit(self): from google.cloud.bigquery.table import TimePartitioningType From ee81b1044efa379e1672751873f23768b144918f Mon Sep 17 00:00:00 2001 From: Tim Swast Date: Wed, 8 May 2019 15:57:03 -0700 Subject: [PATCH 3/4] Remove unused imports --- bigquery/tests/unit/test_table.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/bigquery/tests/unit/test_table.py b/bigquery/tests/unit/test_table.py index c8acff3a076a..07a625b98825 100644 --- a/bigquery/tests/unit/test_table.py +++ b/bigquery/tests/unit/test_table.py @@ -2260,8 +2260,6 @@ def test_constructor_explicit(self): self.assertTrue(time_partitioning.require_partition_filter) def test_from_api_repr_empty(self): - from google.cloud.bigquery.table import TimePartitioningType - klass = self._get_target_class() # Even though there are required properties according to the API @@ -2288,8 +2286,6 @@ def test_from_api_repr_minimal(self): self.assertIsNone(time_partitioning.require_partition_filter) def test_from_api_repr_doesnt_override_type(self): - from google.cloud.bigquery.table import TimePartitioningType - klass = self._get_target_class() api_repr = {"type": "HOUR"} time_partitioning = klass.from_api_repr(api_repr) From daa9c623a1e972aff1ead13b61c34b294d865c0b Mon Sep 17 00:00:00 2001 From: Tim Swast Date: Wed, 8 May 2019 16:02:58 -0700 Subject: [PATCH 4/4] Speed up lint session by installing local deps inplace. --- bigquery/noxfile.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bigquery/noxfile.py b/bigquery/noxfile.py index 39e5f4548c0b..a9df7a67cfcc 100644 --- a/bigquery/noxfile.py +++ b/bigquery/noxfile.py @@ -141,8 +141,10 @@ def lint(session): serious code quality issues. """ - session.install("black", "flake8", *LOCAL_DEPS) - session.install(".") + session.install("black", "flake8") + for local_dep in LOCAL_DEPS: + session.install("-e", local_dep) + session.install("-e", ".") session.run("flake8", os.path.join("google", "cloud", "bigquery")) session.run("flake8", "tests") session.run("flake8", os.path.join("docs", "snippets.py"))