Skip to content

Multiple inequality support #5194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 41 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
7cc549b
initial code
milaGGL Jul 21, 2023
e1a7435
remove validations
milaGGL Jul 21, 2023
e8fd747
update targetIndexMatcher, validation tests
milaGGL Jul 25, 2023
7f55a2c
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Jul 25, 2023
5db48c7
more tests
milaGGL Jul 25, 2023
b37bdd1
format
milaGGL Jul 25, 2023
8ccd669
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Jul 25, 2023
0b605b6
Update CHANGELOG.md
milaGGL Jul 25, 2023
cabb6b8
Update TargetIndexMatcher.java
milaGGL Jul 25, 2023
faa107e
format
milaGGL Jul 25, 2023
468cf50
Update Query.java
milaGGL Jul 26, 2023
ce6b616
Update Query.java
milaGGL Jul 27, 2023
89ddf68
resolve comments
milaGGL Jul 28, 2023
f3c8dae
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Jul 28, 2023
d7ad35f
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 2, 2023
a4dd67f
Revert "Merge branch 'master' into mila/multiple-inequality-support"
milaGGL Aug 2, 2023
b289dae
Revert "Revert "Merge branch 'master' into mila/multiple-inequality-s…
milaGGL Aug 2, 2023
dfbd211
Revert "Revert "Revert "Merge branch 'master' into mila/multiple-ineq…
milaGGL Aug 2, 2023
ad21169
Revert "Revert "Merge branch 'master' into mila/multiple-inequality-s…
milaGGL Aug 2, 2023
9759f40
remove noises from last master merge
milaGGL Aug 2, 2023
1460f26
fix format
milaGGL Aug 2, 2023
439ba28
Update CHANGELOG.md
milaGGL Aug 3, 2023
6ed594a
Simplify code and improve test coverage (#5220)
milaGGL Aug 11, 2023
6bb2c37
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 11, 2023
de360e2
add more unit tests orderby field normalization
milaGGL Aug 17, 2023
3b20dad
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 17, 2023
12de4c0
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 23, 2023
71bfc77
Add compatibility with Cache Index Auto Creation
cherylEnkidu Aug 24, 2023
9f62c29
address feedbacks
cherylEnkidu Aug 29, 2023
19ba7ed
revert rename
cherylEnkidu Aug 30, 2023
a1ac882
add documentation
cherylEnkidu Aug 30, 2023
024da47
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 31, 2023
f565412
address feedback from web review
cherylEnkidu Aug 31, 2023
8d177bf
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Aug 31, 2023
529928e
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Sep 1, 2023
5c7add5
remove the unrelated change in ChangeLog
milaGGL Sep 1, 2023
0a5ed74
add a link
milaGGL Sep 1, 2023
c886203
remove the Changelog for now
milaGGL Sep 1, 2023
bdc7662
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Sep 5, 2023
7b43dde
update test comment
milaGGL Sep 7, 2023
94d28a6
Merge branch 'master' into mila/multiple-inequality-support
milaGGL Sep 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import static com.google.firebase.firestore.Filter.equalTo;
import static com.google.firebase.firestore.Filter.greaterThan;
import static com.google.firebase.firestore.Filter.inArray;
import static com.google.firebase.firestore.Filter.lessThan;
import static com.google.firebase.firestore.Filter.notEqualTo;
import static com.google.firebase.firestore.Filter.notInArray;
import static com.google.firebase.firestore.Filter.or;
import static com.google.firebase.firestore.testutil.Assert.assertThrows;
Expand Down Expand Up @@ -532,41 +534,11 @@ public void queryOrderByKeyBoundsMustBeStringsWithoutSlashes() {
+ "document path, but 'foo' is not because it contains an odd number of segments.");
}

@Test
public void queriesWithDifferentInequalityFieldsFail() {
expectError(
() -> testCollection().whereGreaterThan("x", 32).whereLessThan("y", "cat"),
"All where filters with an inequality (notEqualTo, notIn, lessThan, "
+ "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the "
+ "same field. But you have filters on 'x' and 'y'");
}

@Test
public void queriesWithInequalityDifferentThanFirstOrderByFail() {
CollectionReference collection = testCollection();
String reason =
"Invalid query. You have an inequality where filter (whereLessThan(), "
+ "whereGreaterThan(), etc.) on field 'x' and so you must also have 'x' as "
+ "your first orderBy() field, but your first orderBy() is currently on field 'y' "
+ "instead.";
expectError(() -> collection.whereGreaterThan("x", 32).orderBy("y"), reason);
expectError(() -> collection.orderBy("y").whereGreaterThan("x", 32), reason);
expectError(() -> collection.whereGreaterThan("x", 32).orderBy("y").orderBy("x"), reason);
expectError(() -> collection.orderBy("y").orderBy("x").whereGreaterThan("x", 32), reason);
expectError(() -> collection.orderBy("y").orderBy("x").whereNotEqualTo("x", 32), reason);
}

@Test
public void queriesWithMultipleNotEqualAndInequalitiesFail() {
expectError(
() -> testCollection().whereNotEqualTo("x", 32).whereNotEqualTo("x", 33),
"Invalid Query. You cannot use more than one '!=' filter.");

expectError(
() -> testCollection().whereNotEqualTo("x", 32).whereGreaterThan("y", 33),
"All where filters with an inequality (notEqualTo, notIn, lessThan, "
+ "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the "
+ "same field. But you have filters on 'x' and 'y'");
}

@Test
Expand All @@ -584,9 +556,7 @@ public void queriesWithNotEqualAndNotInFiltersFail() {
public void queriesWithMultipleDisjunctiveFiltersFail() {
expectError(
() -> testCollection().whereNotIn("foo", asList(1, 2)).whereNotIn("bar", asList(1, 2)),
"All where filters with an inequality (notEqualTo, notIn, lessThan, "
+ "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the "
+ "same field. But you have filters on 'foo' and 'bar'");
"Invalid Query. You cannot use more than one 'not_in' filter.");

expectError(
() ->
Expand Down Expand Up @@ -707,37 +677,71 @@ public void queriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray()
}

@Test
public void testInvalidQueryFilters() {
public void testConflictingOperatorsInCompositeFilters() {
CollectionReference collection = testCollection();
// Composite queries can validate conflicting operators in multiple inequality.
String reason = "Invalid Query. You cannot use more than one '!=' filter.";
expectError(
() ->
collection
.where(
or(
and(lessThan("a", "b"), inArray("c", Arrays.asList("d", "e"))),
and(equalTo("e", "f"), notEqualTo("g", "h"))))
.where(
or(
and(greaterThan("i", "j"), greaterThan("k", "l")),
and(notEqualTo("o", "p"), lessThan("q", "r")))),
reason);

// Multiple inequalities, one of which is inside a nested composite filter.
String reason =
"All where filters with an inequality (notEqualTo, notIn, lessThan, lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the same field. But you have filters on 'c' and 'r'";
reason = "Invalid Query. You cannot use more than one 'not_in' filter.";
expectError(
() ->
collection
.where(
or(
and(equalTo("a", "b"), greaterThan("c", "d")),
and(equalTo("e", "f"), equalTo("g", "h"))))
.where(greaterThan("r", "s")),
and(lessThan("a", "b"), notInArray("c", Arrays.asList("d", "e"))),
and(equalTo("e", "f"), lessThan("g", "j"))))
.where(
or(
and(greaterThan("i", "j"), greaterThan("k", "l")),
and(notInArray("o", Arrays.asList("p", "q")), lessThan("q", "r")))),
reason);

// OrderBy and inequality on different fields. Inequality inside a nested composite filter.
reason =
"Invalid query. You have an inequality where filter (whereLessThan(), whereGreaterThan(), etc.) on field 'c' and so you must also have 'c' as your first orderBy() field, but your first orderBy() is currently on field 'r' instead.";
reason = "Invalid Query. You cannot use 'not_in' filters with '!=' filters.";
expectError(
() ->
collection
.where(
or(
and(equalTo("a", "b"), greaterThan("c", "d")),
and(equalTo("e", "f"), equalTo("g", "h"))))
.orderBy("r"),
and(lessThan("a", "b"), greaterThan("c", "d")),
and(equalTo("e", "f"), notEqualTo("g", "h"))))
.where(
or(
and(greaterThan("i", "j"), greaterThan("k", "l")),
and(notInArray("o", Arrays.asList("p", "q")), lessThan("q", "r")))),
reason);

// Conflicting operations within a composite filter.
reason = "Invalid Query. You cannot use 'not_in' filters with 'in' filters.";
expectError(
() ->
collection
.where(
or(
and(lessThan("a", "b"), inArray("c", Arrays.asList("d", "e"))),
and(equalTo("e", "f"), lessThan("g", "j"))))
.where(
or(
and(greaterThan("i", "j"), greaterThan("k", "l")),
and(notInArray("o", Arrays.asList("p", "q")), lessThan("q", "r")))),
reason);
}

@Test
public void testInvalidQueryFilters() {
CollectionReference collection = testCollection();
// Conflicting operations within a composite filter.
String reason = "Invalid Query. You cannot use 'not_in' filters with 'in' filters.";
expectError(
() ->
collection.where(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -487,14 +487,6 @@ private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) {
return parseCompositeFilter((Filter.CompositeFilter) filter);
}

private void validateOrderByField(com.google.firebase.firestore.model.FieldPath field) {
com.google.firebase.firestore.model.FieldPath inequalityField = query.inequalityField();
if (query.getFirstOrderByField() == null && inequalityField != null) {

validateOrderByFieldMatchesInequality(field, inequalityField);
}
}

/**
* Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the
* value is anything other than a DocumentReference or String, or if the string is malformed.
Expand Down Expand Up @@ -544,21 +536,6 @@ private void validateDisjunctiveFilterElements(Object value, Operator op) {
}
}

private void validateOrderByFieldMatchesInequality(
com.google.firebase.firestore.model.FieldPath orderBy,
com.google.firebase.firestore.model.FieldPath inequality) {
if (!orderBy.equals(inequality)) {
String inequalityString = inequality.canonicalString();
throw new IllegalArgumentException(
String.format(
"Invalid query. You have an inequality where filter (whereLessThan(), "
+ "whereGreaterThan(), etc.) on field '%s' and so you must also have '%s' as "
+ "your first orderBy() field, but your first orderBy() is currently on field "
+ "'%s' instead.",
inequalityString, inequalityString, orderBy.canonicalString()));
}
}

/**
* Given an operator, returns the set of operators that cannot be used with it.
*
Expand Down Expand Up @@ -592,24 +569,7 @@ private void validateNewFieldFilter(
com.google.firebase.firestore.core.Query query,
com.google.firebase.firestore.core.FieldFilter fieldFilter) {
Operator filterOp = fieldFilter.getOperator();
if (fieldFilter.isInequality()) {
com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField();
com.google.firebase.firestore.model.FieldPath newInequality = fieldFilter.getField();

if (existingInequality != null && !existingInequality.equals(newInequality)) {
throw new IllegalArgumentException(
String.format(
"All where filters with an inequality (notEqualTo, notIn, lessThan, "
+ "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the "
+ "same field. But you have filters on '%s' and '%s'",
existingInequality.canonicalString(), newInequality.canonicalString()));
}
com.google.firebase.firestore.model.FieldPath firstOrderByField =
query.getFirstOrderByField();
if (firstOrderByField != null) {
validateOrderByFieldMatchesInequality(firstOrderByField, newInequality);
}
}
Operator conflictingOp = findOpInsideFilters(query.getFilters(), conflictingOps(filterOp));
if (conflictingOp != null) {
// We special case when it's a duplicate op to give a slightly clearer error message.
Expand Down Expand Up @@ -717,7 +677,6 @@ private Query orderBy(
"Invalid query. You must not call Query.endAt() or Query.endBefore() before "
+ "calling Query.orderBy().");
}
validateOrderByField(fieldPath);
OrderBy.Direction dir =
direction == Direction.ASCENDING
? OrderBy.Direction.ASCENDING
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import android.text.TextUtils;
import androidx.annotation.Nullable;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.FieldPath;
import com.google.firebase.firestore.util.Function;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -74,19 +73,6 @@ public List<FieldFilter> getFlattenedFilters() {
return Collections.unmodifiableList(memoizedFlattenedFilters);
}

/**
* Returns the first inequality filter contained within this composite filter. Returns {@code
* null} if it does not contain any inequalities.
*/
@Override
public FieldPath getFirstInequalityField() {
FieldFilter found = findFirstMatchingFilter(f -> f.isInequality());
if (found != null) {
return found.getField();
}
return null;
}

public boolean isConjunction() {
return operator == Operator.AND;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,6 @@ public List<Filter> getFilters() {
return Collections.singletonList(this);
}

@Override
public FieldPath getFirstInequalityField() {
if (isInequality()) {
return getField();
}
return null;
}

@Override
public String toString() {
return getCanonicalId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@

package com.google.firebase.firestore.core;

import androidx.annotation.Nullable;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.FieldPath;
import java.util.List;

public abstract class Filter {
Expand All @@ -31,8 +29,4 @@ public abstract class Filter {

/** Returns a list of all filters that are contained within this filter */
public abstract List<Filter> getFilters();

/** Returns the field of the first filter that's an inequality, or null if none. */
@Nullable
public abstract FieldPath getFirstInequalityField();
}
Loading