Skip to content

Conversation

@chemelnucfin
Copy link
Contributor

@chemelnucfin chemelnucfin commented Dec 2, 2017

I need to clean this up a bit, but feel free to have a brief look in case I'm doing something atrocious.

I changed the behavior of a default constructor to include all instead of raising ValueError, which is the same behavior exhibited if you don't specify a end_key.

Also fixed a bug where if an end_key doesn't get specified, it adds the required field before it passes into pb.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2017
@chemelnucfin chemelnucfin changed the title Spanner: Partial range test and bug fix Spanner: Partial range test and bug fix (WIP) Dec 2, 2017
@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch 2 times, most recently from 508d4ae to c3343dd Compare December 4, 2017 05:24
@chemelnucfin chemelnucfin changed the title Spanner: Partial range test and bug fix (WIP) Spanner: Partial range test and bug fix Dec 4, 2017
@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch from c3343dd to 6c01759 Compare December 4, 2017 20:07
@tseaver tseaver mentioned this pull request Dec 4, 2017
19 tasks
@tseaver
Copy link
Contributor

tseaver commented Dec 4, 2017

@chemelnucfin I'm not in favor of KeyRange() as a valid way to spell "include all rows" -- if that was what I wanted, why would I use a range at all?

@chemelnucfin
Copy link
Contributor Author

chemelnucfin commented Dec 4, 2017

@tseaver
Maybe someone needs to pass in a KeyRange into a function expecting that object which selects all rows? Also the default behavior in protobuf is like that, isn't it?

Also, if you don't supply a begin key or an end key, the constructor specifies one for you. So if you don't spply either, it should specify both?

@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch 3 times, most recently from 3c7a227 to cfe9062 Compare December 6, 2017 02:53
@chemelnucfin
Copy link
Contributor Author

@tseaver PTAL, thanks.

@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch from cfe9062 to 8f1ebe9 Compare December 6, 2017 05:57
@tseaver
Copy link
Contributor

tseaver commented Dec 6, 2017

@chemelnucfin I'm still opposed to dropping the ValueError in the constructor: application code is responsible for constructing the KeySet instance which contains the KeyRange: if it wants to include all values, there are easier ways to spell that, e.g. KeySet(all_=True).

@chemelnucfin
Copy link
Contributor Author

@tseaver My main concern is that there is a difference in behavior if you don't specify either a start or an end key, it gets filled out automatically, and if you don't specify both start and end key it raises ValueError. I think either both cases should raise ValueError, or both should get them filled out automatically.

@tseaver
Copy link
Contributor

tseaver commented Dec 6, 2017

Specifying a range with only one of start_* or end_* has well-defined semantics, which we have no other way to spell. Specifying a range with not bounds at all is just a confusing / potentially expensive way to as for the same rows as KeySet(all_=True)1.

@lukesneeringer, @dhermes Can one of you chime in?

@dhermes
Copy link
Contributor

dhermes commented Dec 6, 2017

ISTM @tseaver is right on here.

@chemelnucfin
Copy link
Contributor Author

I still somewhat disagree, since a KeyRange object shouldn't care what a KeySet does with it. If anything, the check should be in KeySet to convert an empty KeyRange into all_=True. The behavior within KeyRange is still inconsistent with not supplying missing keys.

However, I can understand your point that you shouldn't construct a plain KeyRange object.

Anyway, my suggestion would be, either to document this behavior, or convert an empty KeyRange object into all_=True in KeySet.

@chemelnucfin
Copy link
Contributor Author

BTW, I did change the empty constructor back to the way it was.

@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch from bba3420 to 1b76a11 Compare December 13, 2017 20:20
kwargs['start_open'] = _make_list_value_pb(self.start_open)

if self.start_closed:
elif self.start_closed is not None:

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

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([])

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@chemelnucfin chemelnucfin force-pushed the spanner_test_partial_keys_range_with_index branch from 1b76a11 to e181398 Compare December 18, 2017 23:40
@chemelnucfin
Copy link
Contributor Author

Closing in favor of #4618 and #4623. Maybe revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants