feat: OR Queries #1800
feat: OR Queries #1800
Conversation
product-auto-label
bot
added
size: l
MarkDuckworth
changed the title
MarkDuckworth
added
the
do not merge
…rkduckworth/or-queries
* OR operator support and integration tests. * Updating documentation and member visibility. * Remove usage of OPERATOR_UNSPECIFIED standing in for OR. * Removing private and internal tags from OR query public API members. * Ensure that new OR Query features are in firestore.d.ts and are exported from the main entry point. * Update documentation for OR query features to match android PR 4274. * Removing CompositeFilter and UnaryFilter from the type definitions and JS doc. * Corrected the descending order test for OR queries. * fix: update generated proto types; fix the update script (#1825) * Adding OR enum value for composit filter operator. --------- Co-authored-by: Alexander Fenster <[email protected]>
product-auto-label
bot
added
size: xl
MarkDuckworth
changed the title
MarkDuckworth
requested review from
ehsannas
and removed request for
ehsannas
dev/system-test/firestore.ts
Outdated
| expectDocs( | ||
| await collection | ||
| .where( | ||
| Filter.or( | ||
| Filter.where('a', '==', 2), | ||
| Filter.where('b', 'in', [2, 3]) | ||
| ) | ||
| ) | ||
| .get(), | ||
| 'doc3', | ||
| 'doc4', | ||
| 'doc6' | ||
| ); |
There was a problem hiding this comment.
This one doesn't need a composite index.
so you can break it to 2 tests
supports OR queries with in --> doesn't need composite index (because it doesn't have inequality)
supports OR queries with not-in --> needs composite index.
There was a problem hiding this comment.
Split this. Thanks.
MarkDuckworth
added
kokoro:run
yoshi-kokoro
removed
kokoro:run
ehsannas
removed
the
do not merge
| * }); | ||
| * ``` | ||
| */ | ||
| public static or(...filters: Filter[]): Filter { |
There was a problem hiding this comment.
I implemented these static functions outside of the filter class so that they could be used without prefixing with Filter.or or Filter.and for instance.
There was a problem hiding this comment.
This came up in API design review and we decided to go with this approach because it is more consistent with existing static functions in the nodejs-firestore SDK. See for example, the FieldValue class.
| * @private | ||
| * @internal | ||
| */ | ||
| export class UnaryFilter extends Filter { |
There was a problem hiding this comment.
I think we want to call this a PropertyFilter to be consistent with the documentation.
There was a problem hiding this comment.
UnaryFilter was ported from Android, which I need to stay consistent with. This class is not public, so we could conceivably change the name in the future.
| } | ||
|
|
||
| public toProto(): api.StructuredQuery.IFilter { | ||
| if (this.filters.length === 1) { |
There was a problem hiding this comment.
Just a note here that for the datastore PR, we still use an AND filter with one element because it fits in naturally with the way that the code already is.
There was a problem hiding this comment.
That sounds good. I think that's also acceptable. This particular implementation was done to be consistent with other Firestore SDK implementations (ported from Android SDK).
| where( | ||
| fieldPathOrFilter: string | firestore.FieldPath | Filter, | ||
| opStr?: firestore.WhereFilterOp, | ||
| value?: unknown | ||
| ): Query<T> { |
There was a problem hiding this comment.
Just a note that this is like the .filter function in datastore
There was a problem hiding this comment.
I'm not sure on the history, but I agree the naming conventions deviate between Firestore and Datastore.
| } | ||
| return new CompositeFilterInternal( | ||
| parsedFilters, | ||
| compositeFilterData._getOperator() === 'AND' ? 'AND' : 'OR' |
There was a problem hiding this comment.
Should we throw an error here instead if OR or AND is not provided?
@danieljbruce I'm not sure of the history of this, but I have an open ticket to modify this SDK so that we use the generated d.ts files. See b/269761436 |
Adding support for OR queries