Tighten the query bounds #2979
Tighten the query bounds #2979
Conversation
This PR implements type clamping. A query for foo < 3 should only return integers, so we know both a min value and a max value.
| @@ -34,7 +34,7 @@ | |||
| public static final int INDEX_TYPE_BOOLEAN = 10; | |||
| public static final int INDEX_TYPE_NAN = 13; | |||
| public static final int INDEX_TYPE_NUMBER = 15; | |||
| public static final int INDEX_TYPE_TIMESTAMP = 10; | |||
| public static final int INDEX_TYPE_TIMESTAMP = 20; | |||
There was a problem hiding this comment.
This was a typo.
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (0dcb09f8) is created by Prow via merging commits: da62838 82f497f. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (0dcb09f8) is created by Prow via merging commits: da62838 82f497f. |
| for (Filter filter : filters) { | ||
| if (filter.getField().equals(segment.getFieldPath())) { | ||
| FieldFilter fieldFilter = (FieldFilter) filter; | ||
|
|
||
| Value newValue = null; |
There was a problem hiding this comment.
optional: s/new/current for here and below?
When looking at newValue, I initially thought that this value was going to be assigned because of the new, but it seems like it's only evaluated to be compared to the outer lowestValue.
There was a problem hiding this comment.
Will update.
| @@ -432,4 +457,64 @@ public static Value refValue(DatabaseId databaseId, DocumentKey key) { | |||
| .build(); | |||
| return value; | |||
| } | |||
|
|
|||
| public static Value getFirstValue(Value.ValueTypeCase valueTypeCase) { | |||
There was a problem hiding this comment.
Will rename.
| } | ||
| } | ||
|
|
||
| public static @Nullable Value getNextValue(Value.ValueTypeCase valueTypeCase) { |
There was a problem hiding this comment.
s/getNextValue/getHighestValue or getHighestPossibleValue?
NextValue is a bit confusing here. Alternatively, just add some code comments please.
There was a problem hiding this comment.
Looks like I forgot to push the commit with the comments. I agree that this is not a great name - however this method returns an exclusive value, i.e. one higher than the max. Do you have a name suggestion that conveys that? I need your input here :)
There was a problem hiding this comment.
Some ideas: getHighestValueExclusive, getUpperBoundExclusive, getExclusiveUpperBound
I think pairing "exclusive" with a "bound" makes more sense. Or you can leave the current name and just have comments.
There was a problem hiding this comment.
Changed to getLowerBound() and getUpperBound(). The comments still state whether it is exclusive or not.
| @@ -143,14 +143,14 @@ public void testStartAfterFilter() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testEndAtFilter() { | |||
| public void testEndBeforeFilter() { | |||
There was a problem hiding this comment.
I'm a little confused about the before in Bound. Based on the naming of this test, if before == true, Bound does not include the endAt value? However, it seems like it's flipped for startAt and startAfter.
I find a it harder to reason about before/after since it's flipped for whether it's a startAt, or endAt. Is it possible to change it before to an inclusive/exclusive model instead?
There was a problem hiding this comment.
It's super confusing, but it uses the existing backend terminology:
startAt => before is true (cursor starts right before the provided value)
startAfter => before is false (cursor starts right after the provided value)
endBefore => before is false (cursor stops right before the provided value)
endAt => before is true (cursor stops right after the provided value)
I am confused by this myself, hence the test name here is wrong.
There was a problem hiding this comment.
oh man this still doesn't make too much sense... Still can't wrap my mind around how endBefore has before set to false in order to mean that the cursor stops before??
Is this documented anywhere? optional nice thing you can do is encapsulate the before bound stuff into some tester objects :)
There was a problem hiding this comment.
Will clean up in a follow up.
This PR implements type clamping. A query for foo < 3 should only return integers, so we know both a min value and a max value.