Spanner: Partial range test and bug fix

[chemelnucfin]

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.

[tseaver]

@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]

@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]

@tseaver PTAL, thanks.

[tseaver]

@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]

@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]

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]

ISTM @tseaver is right on here.

[chemelnucfin]

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]

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

[chemelnucfin]

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