Spanner: partial key range and bug fix with single key #4618

chemelnucfin · 2017-12-18T23:25:05Z

breaking up #4515

spanner/google/cloud/spanner_v1/keyset.py

+        elif self.start_closed is not None:
            kwargs['start_closed'] = _make_list_value_pb(self.start_closed)
+        else:
+            kwargs['start_closed'] = _make_list_value_pb([])


spanner/tests/unit/test_keyset.py


 import unittest

+import six


spanner/tests/unit/test_keyset.py

            self._make_one(end_open=KEY_1, end_closed=KEY_2)

-    def test_ctor_w_only_start_open(self):
+    def test_ctor_start_open_and_start_closed_empty_lists_and_end_closed(self):


spanner/tests/unit/test_keyset.py

+        with self.assertRaises(ValueError):
+            self._make_one(start_open=KEY_1, end_open=[], end_closed=[])
+
+    def _check_unused_keys(self, krange, used_keys):


spanner/tests/unit/test_keyset.py

+                     'end_open')
+        for key_type in key_types:
+            if key_type not in used_keys:
+                self.assertFalse(krange.to_pb().HasField(key_type))


spanner/tests/unit/test_keyset.py

-        self.assertEqual(krange.end_closed, None)
+        KEY_2 = [u'key_2']
+        with self.assertRaises(ValueError):
+            self._make_one(start_open=[], start_closed=[], end_closed=KEY_1)


spanner/tests/unit/test_keyset.py

+            self._make_one(start_open=[], start_closed=[], end_closed=KEY_1)

-    def test_ctor_w_only_start_closed(self):
+    def test_ctor_start_open_and_end_open_and_end_closed_empty_lists(self):


spanner/tests/unit/test_keyset.py

+        self.assertIsNone(krange.start_closed)
+        self.assertIsNone(krange.end_open)
+
+    def test_ctor_single_key_end_open(self):


spanner/tests/unit/test_keyset.py

+        self.assertEqual(pb_key_type.values[0].string_value, KEY_1[0])
+        self._check_unused_keys(krange, used_keys)
+
+    def test_to_pb_single_key_end_open(self):


spanner/google/cloud/spanner_v1/keyset.py

    """
    def __init__(self, start_open=None, start_closed=None,
                 end_open=None, end_closed=None):
        if not any([start_open, start_closed, end_open, end_closed]):


spanner/google/cloud/spanner_v1/keyset.py

+        if start_open is not None and start_closed is not None:
            raise ValueError("Specify one of 'start_open' / 'start_closed'.")
+        elif start_open is None and start_closed is None:
+            start_closed = []


spanner/google/cloud/spanner_v1/keyset.py

-            raise ValueError("Must specify at least a start or end row.")
-
-        if start_open and start_closed:
+        if start_open is not None and start_closed is not None:


chemelnucfin · 2017-12-19T03:46:09Z

maybe breaking this up and changing the code all at once isn't such a great idea.....

chemelnucfin · 2017-12-19T03:46:22Z

working on it...

spanner/google/cloud/spanner_v1/keyset.py

                 end_open=None, end_closed=None):
-        if not any([start_open, start_closed, end_open, end_closed]):
+        keys = (start_open, start_closed, end_open, end_closed)
+        if all(key is None for key in keys):


spanner/google/cloud/spanner_v1/keyset.py

+        if start_open is not None and start_closed is not None:
            raise ValueError("Specify one of 'start_open' / 'start_closed'.")
+        elif start_open is None and start_closed is None:
+            start_closed = []


spanner/google/cloud/spanner_v1/keyset.py

            kwargs['start_open'] = _make_list_value_pb(self.start_open)
-
-        if self.start_closed:
+        else:


spanner/tests/unit/test_keyset.py

            self._make_one()

-    def test_ctor_w_start_open_and_start_closed(self):
+    def test_ctor_start_open_and_start_closed(self):


spanner/tests/unit/test_keyset.py

+        with self.assertRaises(ValueError):
+            self._make_one(start_open=KEY_1, end_open=[], end_closed=[])
+
+    def _check_unused_keys(self, key_range, used_keys):


spanner/tests/unit/test_keyset.py

+        self.assertIsNone(krange.start_open)
+        self.assertIsNone(krange.end_open)
+
+    def test_ctor_single_key_end_open(self):


chemelnucfin · 2017-12-19T17:57:12Z

@dhermes PTAL, thanks.

spanner/google/cloud/spanner_v1/keyset.py

-            raise ValueError("Must specify at least a start or end row.")
+        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 start key type")


chemelnucfin · 2017-12-19T20:20:44Z

no opposition, changed.

spanner/google/cloud/spanner_v1/keyset.py

-        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 is None and start_closed is None)
+            or (start_open is not None and start_closed is not None)):


spanner/tests/unit/test_keyset.py

+
+        KEY_1 = [u'key_1']
+        with self.assertRaises(ValueError):
+            krange = self._make_one(start_closed=KEY_1)


- Converting to "Google" docstring style - Adding note about the "exactly one of" requirement with a suggestion about what to pass if the start/end is not important to the user - Adding a `Raises` section to the class docstring

Also reworked the `to_pb()` tests to just create a protobuf and just use one assertion.

googlebot · 2017-12-20T18:00:00Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dhermes

LGTM

This is a follow-on to #4618.

…assed to KeyRange (#4618)" This reverts commit 4d6cd26.

#4733) Revert "Spanner: Make sure **exactly** one of `start_*`/`end_*` are passed to KeyRange (#4618)" This reverts commit 4d6cd26.

chemelnucfin requested a review from lukesneeringer as a code owner December 18, 2017 23:25

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 18, 2017

dhermes reviewed Dec 18, 2017

View reviewed changes

dhermes reviewed Dec 19, 2017

View reviewed changes

chemelnucfin force-pushed the spanner_only_key_bugfix branch from 9954202 to 63b8948 Compare December 19, 2017 01:14

chemelnucfin force-pushed the spanner_only_key_bugfix branch 2 times, most recently from 762322c to ba5619c Compare December 19, 2017 07:55

chemelnucfin mentioned this pull request Dec 19, 2017

Spanner: Partial range test and bug fix #4515

Closed

chemelnucfin force-pushed the spanner_only_key_bugfix branch from ba5619c to 9283a46 Compare December 19, 2017 08:01

dhermes reviewed Dec 19, 2017

View reviewed changes

chemelnucfin force-pushed the spanner_only_key_bugfix branch 3 times, most recently from 020091d to 8d84c34 Compare December 19, 2017 17:56

dhermes reviewed Dec 19, 2017

View reviewed changes

chemelnucfin force-pushed the spanner_only_key_bugfix branch from 8d84c34 to 4192357 Compare December 19, 2017 20:15

dhermes reviewed Dec 19, 2017

View reviewed changes

chemelnucfin mentioned this pull request Dec 19, 2017

Spanner: System test partial range #4631

Merged

dhermes reviewed Dec 19, 2017

View reviewed changes

spanner/tests/unit/test_keyset.py Outdated

KEY_1 = [u'key_1']

with self.assertRaises(ValueError):

krange = self._make_one(start_closed=KEY_1)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

chemelnucfin force-pushed the spanner_only_key_bugfix branch from e973bd7 to a106cda Compare December 20, 2017 01:01

chemelnucfin added 6 commits December 20, 2017 09:59

Spanner: partial key range and bug fix with single key

05d64d7

Review Change

7f98aeb

Spanner: Keyset change single key to be not allowed

82db911

Review Change

08d50b8

Spanner: Review Changes

a5de006

remove krange

ccbb29a

dhermes added 4 commits December 20, 2017 09:59

Removing KeyRange.to_pb() tests that don't actually invoke to_pb().

17406f9

Updating KeyRange docstring.

ad8e30e

- Converting to "Google" docstring style - Adding note about the "exactly one of" requirement with a suggestion about what to pass if the start/end is not important to the user - Adding a `Raises` section to the class docstring

Changing truthy checks to not None checks in KeyRange.to_pb().

31b1201

Adding a unit test with the empty list case in KeyRange.to_pb().

88ca5bb

Also reworked the `to_pb()` tests to just create a protobuf and just use one assertion.

dhermes force-pushed the spanner_only_key_bugfix branch from a106cda to 88ca5bb Compare December 20, 2017 17:59

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 20, 2017

dhermes approved these changes Dec 20, 2017

View reviewed changes

dhermes merged commit 4d6cd26 into googleapis:master Dec 20, 2017

dhermes pushed a commit that referenced this pull request Dec 20, 2017

Spanner: Adding system test for "partial" key ranges (#4631)

157b498

This is a follow-on to #4618.

This was referenced Jan 4, 2018

KeyRange arguments change is backward-incompatible #4694

Closed

Release Spanner v0.30.0 #4732

Merged

theacodes pushed a commit that referenced this pull request Jan 10, 2018

Revert "Spanner: Make sure **exactly** one of start_*/end_* are p…

b8c9ee6

…assed to KeyRange (#4618)" This reverts commit 4d6cd26.

theacodes mentioned this pull request Jan 10, 2018

Revert "Spanner: Make sure **exactly** one of start_*/end_* are p… #4733

Merged

theacodes pushed a commit that referenced this pull request Jan 11, 2018

Revert "Spanner: Make sure **exactly** one of start_*/end_* are p… (

36c7a16

#4733) Revert "Spanner: Make sure **exactly** one of `start_*`/`end_*` are passed to KeyRange (#4618)" This reverts commit 4d6cd26.

Spanner: partial key range and bug fix with single key #4618

Spanner: partial key range and bug fix with single key #4618

Uh oh!

Conversation

chemelnucfin commented Dec 18, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

chemelnucfin commented Dec 19, 2017

Uh oh!

chemelnucfin commented Dec 19, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

chemelnucfin commented Dec 19, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

chemelnucfin commented Dec 19, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!