From b8c9ee64e660274a1682f90ff5787c6b5e11d527 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Wed, 10 Jan 2018 15:42:15 -0800 Subject: [PATCH 1/2] Revert "Spanner: Make sure **exactly** one of `start_*`/`end_*` are passed to KeyRange (#4618)" This reverts commit 4d6cd26c32ede99afd36ec96558c54d6120791f4. --- spanner/google/cloud/spanner_v1/keyset.py | 56 +++++------ spanner/tests/unit/test_keyset.py | 115 +++++++++------------- 2 files changed, 67 insertions(+), 104 deletions(-) diff --git a/spanner/google/cloud/spanner_v1/keyset.py b/spanner/google/cloud/spanner_v1/keyset.py index a5512ec28ca1..72b849fe5cf1 100644 --- a/spanner/google/cloud/spanner_v1/keyset.py +++ b/spanner/google/cloud/spanner_v1/keyset.py @@ -24,40 +24,28 @@ class KeyRange(object): """Identify range of table rows via start / end points. - .. note:: - - Exactly one of ``start_open`` and ``start_closed`` must be - passed and exactly one of ``end_open`` and ``end_closed`` must be. - To "start at the beginning" (i.e. specify no start for the range) - pass ``start_closed=[]``. To "go to the end" (i.e. specify no end - for the range) pass ``end_closed=[]``. - - Args: - start_open (List): Keys identifying start of range (this key - excluded). - start_closed (List): Keys identifying start of range (this key - included). - end_open (List): Keys identifying end of range (this key - excluded). - end_closed (List): Keys identifying end of range (this key - included). - - Raises: - ValueError: If **neither** ``start_open`` or ``start_closed`` is - passed. - ValueError: If **both** ``start_open`` and ``start_closed`` are passed. - ValueError: If **neither** ``end_open`` or ``end_closed`` is passed. - ValueError: If **both** ``end_open`` and ``end_closed`` are passed. + :type start_open: list of scalars + :param start_open: keys identifying start of range (this key excluded) + + :type start_closed: list of scalars + :param start_closed: keys identifying start of range (this key included) + + :type end_open: list of scalars + :param end_open: keys identifying end of range (this key excluded) + + :type end_closed: list of scalars + :param end_closed: keys identifying end of range (this key included) """ def __init__(self, start_open=None, start_closed=None, end_open=None, end_closed=None): - if ((start_open is None and start_closed is None) - or (start_open is not None and start_closed is not None)): - raise ValueError('Specify exactly one of start_closed or start_open') + if not any([start_open, start_closed, end_open, end_closed]): + raise ValueError("Must specify at least a start or end row.") + + if start_open and start_closed: + raise ValueError("Specify one of 'start_open' / 'start_closed'.") - if ((end_open is None and end_closed is None) - or (end_open is not None and end_closed is not None)): - raise ValueError('Specify exactly one of end_closed or end_open') + if end_open and end_closed: + raise ValueError("Specify one of 'end_open' / 'end_closed'.") self.start_open = start_open self.start_closed = start_closed @@ -72,16 +60,16 @@ def to_pb(self): """ kwargs = {} - if self.start_open is not None: + if self.start_open: kwargs['start_open'] = _make_list_value_pb(self.start_open) - if self.start_closed is not None: + if self.start_closed: kwargs['start_closed'] = _make_list_value_pb(self.start_closed) - if self.end_open is not None: + if self.end_open: kwargs['end_open'] = _make_list_value_pb(self.end_open) - if self.end_closed is not None: + if self.end_closed: kwargs['end_closed'] = _make_list_value_pb(self.end_closed) return KeyRangePB(**kwargs) diff --git a/spanner/tests/unit/test_keyset.py b/spanner/tests/unit/test_keyset.py index 37ffe82f3cd7..8ff68d81d3cd 100644 --- a/spanner/tests/unit/test_keyset.py +++ b/spanner/tests/unit/test_keyset.py @@ -30,47 +30,49 @@ def test_ctor_no_start_no_end(self): with self.assertRaises(ValueError): self._make_one() - def test_ctor_start_open_and_start_closed(self): + def test_ctor_w_start_open_and_start_closed(self): KEY_1 = [u'key_1'] KEY_2 = [u'key_2'] with self.assertRaises(ValueError): self._make_one(start_open=KEY_1, start_closed=KEY_2) - def test_ctor_end_open_and_end_closed(self): + def test_ctor_w_end_open_and_end_closed(self): KEY_1 = [u'key_1'] KEY_2 = [u'key_2'] with self.assertRaises(ValueError): self._make_one(end_open=KEY_1, end_closed=KEY_2) - def test_ctor_conflicting_start(self): + def test_ctor_w_only_start_open(self): KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(start_open=[], start_closed=[], end_closed=KEY_1) - - def test_ctor_conflicting_end(self): - KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(start_open=KEY_1, end_open=[], end_closed=[]) - - def test_ctor_single_key_start_closed(self): - KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(start_closed=KEY_1) + krange = self._make_one(start_open=KEY_1) + self.assertEqual(krange.start_open, KEY_1) + self.assertEqual(krange.start_closed, None) + self.assertEqual(krange.end_open, None) + self.assertEqual(krange.end_closed, None) - def test_ctor_single_key_start_open(self): + def test_ctor_w_only_start_closed(self): KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(start_open=KEY_1) + krange = self._make_one(start_closed=KEY_1) + self.assertEqual(krange.start_open, None) + self.assertEqual(krange.start_closed, KEY_1) + self.assertEqual(krange.end_open, None) + self.assertEqual(krange.end_closed, None) - def test_ctor_single_key_end_closed(self): + def test_ctor_w_only_end_open(self): KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(end_closed=KEY_1) + krange = self._make_one(end_open=KEY_1) + self.assertEqual(krange.start_open, None) + self.assertEqual(krange.start_closed, None) + self.assertEqual(krange.end_open, KEY_1) + self.assertEqual(krange.end_closed, None) - def test_ctor_single_key_end_open(self): + def test_ctor_w_only_end_closed(self): KEY_1 = [u'key_1'] - with self.assertRaises(ValueError): - self._make_one(end_open=KEY_1) + krange = self._make_one(end_closed=KEY_1) + self.assertEqual(krange.start_open, None) + self.assertEqual(krange.start_closed, None) + self.assertEqual(krange.end_open, None) + self.assertEqual(krange.end_closed, KEY_1) def test_ctor_w_start_open_and_end_closed(self): KEY_1 = [u'key_1'] @@ -91,58 +93,31 @@ def test_ctor_w_start_closed_and_end_open(self): self.assertEqual(krange.end_closed, None) def test_to_pb_w_start_closed_and_end_open(self): - from google.protobuf.struct_pb2 import ListValue - from google.protobuf.struct_pb2 import Value from google.cloud.spanner_v1.proto.keys_pb2 import KeyRange - key1 = u'key_1' - key2 = u'key_2' - key_range = self._make_one(start_closed=[key1], end_open=[key2]) - key_range_pb = key_range.to_pb() - expected = KeyRange( - start_closed=ListValue(values=[ - Value(string_value=key1) - ]), - end_open=ListValue(values=[ - Value(string_value=key2) - ]), - ) - self.assertEqual(key_range_pb, expected) + KEY_1 = [u'key_1'] + KEY_2 = [u'key_2'] + krange = self._make_one(start_closed=KEY_1, end_open=KEY_2) + krange_pb = krange.to_pb() + self.assertIsInstance(krange_pb, KeyRange) + self.assertEqual(len(krange_pb.start_closed), 1) + self.assertEqual(krange_pb.start_closed.values[0].string_value, + KEY_1[0]) + self.assertEqual(len(krange_pb.end_open), 1) + self.assertEqual(krange_pb.end_open.values[0].string_value, KEY_2[0]) def test_to_pb_w_start_open_and_end_closed(self): - from google.protobuf.struct_pb2 import ListValue - from google.protobuf.struct_pb2 import Value - from google.cloud.spanner_v1.proto.keys_pb2 import KeyRange - - key1 = u'key_1' - key2 = u'key_2' - key_range = self._make_one(start_open=[key1], end_closed=[key2]) - key_range_pb = key_range.to_pb() - expected = KeyRange( - start_open=ListValue(values=[ - Value(string_value=key1) - ]), - end_closed=ListValue(values=[ - Value(string_value=key2) - ]), - ) - self.assertEqual(key_range_pb, expected) - - def test_to_pb_w_empty_list(self): - from google.protobuf.struct_pb2 import ListValue - from google.protobuf.struct_pb2 import Value from google.cloud.spanner_v1.proto.keys_pb2 import KeyRange - key = u'key' - key_range = self._make_one(start_closed=[], end_closed=[key]) - key_range_pb = key_range.to_pb() - expected = KeyRange( - start_closed=ListValue(values=[]), - end_closed=ListValue(values=[ - Value(string_value=key) - ]), - ) - self.assertEqual(key_range_pb, expected) + KEY_1 = [u'key_1'] + KEY_2 = [u'key_2'] + krange = self._make_one(start_open=KEY_1, end_closed=KEY_2) + krange_pb = krange.to_pb() + self.assertIsInstance(krange_pb, KeyRange) + self.assertEqual(len(krange_pb.start_open), 1) + self.assertEqual(krange_pb.start_open.values[0].string_value, KEY_1[0]) + self.assertEqual(len(krange_pb.end_closed), 1) + self.assertEqual(krange_pb.end_closed.values[0].string_value, KEY_2[0]) class TestKeySet(unittest.TestCase): From 9f421855bb4d193cdf675b082d877c1d83b838b5 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Wed, 10 Jan 2018 16:21:50 -0800 Subject: [PATCH 2/2] Temporarily fix --- spanner/google/cloud/spanner_v1/keyset.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spanner/google/cloud/spanner_v1/keyset.py b/spanner/google/cloud/spanner_v1/keyset.py index 72b849fe5cf1..cafa3958cb61 100644 --- a/spanner/google/cloud/spanner_v1/keyset.py +++ b/spanner/google/cloud/spanner_v1/keyset.py @@ -60,16 +60,16 @@ def to_pb(self): """ kwargs = {} - if self.start_open: + if self.start_open is not None: kwargs['start_open'] = _make_list_value_pb(self.start_open) - if self.start_closed: + if self.start_closed is not None: kwargs['start_closed'] = _make_list_value_pb(self.start_closed) - if self.end_open: + if self.end_open is not None: kwargs['end_open'] = _make_list_value_pb(self.end_open) - if self.end_closed: + if self.end_closed is not None: kwargs['end_closed'] = _make_list_value_pb(self.end_closed) return KeyRangePB(**kwargs)