Port Filter/Target changes #5929
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (648,103 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
|
Sorry I've fallen behind on this. I'll take a look soon. |
| left: Value | undefined, | ||
| right: Value | undefined | ||
| ): Value | undefined { | ||
| if (left === undefined && right === undefined) { |
There was a problem hiding this comment.
nit: the first if block is not needed. Same for valuesMin
There was a problem hiding this comment.
Had to think about this for a bit but you are right. Fixed.
|
|
||
| /** | ||
| * Returns a lower bound of field values that can be used as a starting point to | ||
| * scan the index defined by `fieldIndex`}`. Returns `null` if no lower bound |
| @@ -513,6 +771,7 @@ export class ArrayContainsAnyFilter extends FieldFilter { | |||
| } | |||
| } | |||
|
|
|||
| // TODO(indexing): Change Bound.before to "inclusive" | |||
There was a problem hiding this comment.
+1. I found inclusive/exclusive terminology easier to understand than before/!before.
| case Operator.NOT_EQUAL: | ||
| // NotIn/NotEqual is always a suffix | ||
| values.push(fieldFilter.value); | ||
| return values; |
There was a problem hiding this comment.
Is it guaranteed that EQUAL and IN come before NOT_IN and NOT_EQUAL during these iterations?
I know this is the same way the code is written in the Android SDK. I'm just curious where does this guarantee come from? and I'm thinking whether this will continue to work for sub-targets that we create for dnf terms in the future.
No action needed for this PR.
There was a problem hiding this comment.
I had to think about this a bit when I ported (and also recently added a unit test for it - https://github.com/firebase/firebase-android-sdk/pull/3351/files#diff-216e85d9cb7dc6a9ca6b6bf1ae2d9e7751d20a13802e9a7c97a8f517b0bff67cR128). The index encoding order is not specified by the query but rather the index. The order that the values is encoded in is specific to the index.
There was a problem hiding this comment.
I will try to improve the comment.
This is a pretty liberal port of https://github.com/firebase/firebase-android-sdk/pull/3237/files that also brings in a bunch of Indexing functionality from the Android SDK (as the PR touches a bunch of this functionality).
The PR also cleans up an existing naming issue of
isDocumentTargetand renames it to follow the pattern className+methodName. In the tests, the PR simplifies theboundmethod.