Merge branch 'main' into PR #2165 to update

Author: lqiu96

google-cloud-firestore/src/main/java/com/google/cloud/firestore/PipelineUtils.java

@@ -108,7 +108,7 @@ static BooleanExpression toPipelineBooleanExpr(FilterInternal f) {
         case EQUAL:
           return and(field.exists(), field.equal(value));
         case NOT_EQUAL:
-          return and(field.exists(), field.notEqual(value));
+          return field.notEqual(value);
         case ARRAY_CONTAINS:
           return and(field.exists(), field.arrayContains(value));
         case IN:
@@ -119,8 +119,7 @@ static BooleanExpression toPipelineBooleanExpr(FilterInternal f) {
           return and(field.exists(), arrayContainsAny(field, Lists.newArrayList(valuesListAny)));
         case NOT_IN:
           List<Value> notInValues = value.getArrayValue().getValuesList();
-          return and(
-              field.exists(), not(Expression.equalAny(field, Lists.newArrayList(notInValues))));
+          return not(Expression.equalAny(field, Lists.newArrayList(notInValues)));
         default:
           // Handle OPERATOR_UNSPECIFIED and UNRECOGNIZED cases as needed
           throw new IllegalArgumentException("Unsupported operator: " + comparisonFilter.operator);

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

@@ -373,21 +373,30 @@ public boolean equals(Object o) {
       return Objects.equals(fieldReference, filter.fieldReference);
     }
 
-    public int compare(QueryDocumentSnapshot doc1, QueryDocumentSnapshot doc2) {
-      String path = fieldReference.getFieldPath();
-      if (FieldPath.isDocumentId(path)) {
-        return direction.documentIdComparator.compare(doc1, doc2);
+    @Override
+    public int compare(QueryDocumentSnapshot left, QueryDocumentSnapshot right) {
+      Value leftValue =
+          left.extractField(FieldPath.fromDotSeparatedString(fieldReference.getFieldPath()));
+      Value rightValue =
+          right.extractField(FieldPath.fromDotSeparatedString(fieldReference.getFieldPath()));
+
+      // If the field isn't present, we treat it as a null value.
+      if (leftValue == null) {
+        leftValue =
+            Value.newBuilder().setNullValue(com.google.protobuf.NullValue.NULL_VALUE).build();
+      }
+      if (rightValue == null) {
+        rightValue =
+            Value.newBuilder().setNullValue(com.google.protobuf.NullValue.NULL_VALUE).build();
       }
-      FieldPath fieldPath = FieldPath.fromDotSeparatedString(path);
-      Preconditions.checkState(
-          doc1.contains(fieldPath) && doc2.contains(fieldPath),
-          "Can only compare fields that exist in the DocumentSnapshot."
-              + " Please include the fields you are ordering on in your select() call.");
-      Value v1 = doc1.extractField(fieldPath);
-      Value v2 = doc2.extractField(fieldPath);
 
-      int cmp = com.google.cloud.firestore.Order.INSTANCE.compare(v1, v2);
-      return (direction == Direction.ASCENDING) ? cmp : -cmp;
+      int cmp = com.google.cloud.firestore.Order.INSTANCE.compare(leftValue, rightValue);
+
+      if (direction == Direction.DESCENDING) {
+        cmp = -cmp;
+      }
+
+      return cmp;
     }
   }
 
@@ -2052,6 +2061,7 @@ Pipeline pipeline() {
     }
 
     // Orders
+    List<FieldOrder> explicitSortOrder = this.options.getFieldOrders();
     List<FieldOrder> normalizedOrderBy = createImplicitOrderBy();
     int size = normalizedOrderBy.size();
     List<Field> fields = new ArrayList<>(size);
@@ -2066,14 +2076,25 @@ Pipeline pipeline() {
       }
     }
 
-    if (fields.size() == 1) {
-      ppl = ppl.where(fields.get(0).exists());
-    } else {
+    // Only add existence filters for fields that are explicitly ordered.
+    // Implicit order bys (e.g. from inequality filters) should not generate
+    // existence filters
+    // because the inequality filter itself will already generate an existence
+    // filter if needed,
+    // or arguably should not if it's a NOT_IN / NOT_EQUAL filter.
+    List<Field> existenceCheckFields = new ArrayList<>();
+    for (FieldOrder order : explicitSortOrder) {
+      existenceCheckFields.add(Field.ofServerPath(order.fieldReference.getFieldPath()));
+    }
+
+    if (existenceCheckFields.size() == 1) {
+      ppl = ppl.where(existenceCheckFields.get(0).exists());
+    } else if (existenceCheckFields.size() > 1) {
       ppl =
           ppl.where(
               and(
-                  fields.get(0).exists(),
-                  fields.subList(1, fields.size()).stream()
+                  existenceCheckFields.get(0).exists(),
+                  existenceCheckFields.subList(1, existenceCheckFields.size()).stream()
                       .map((Field field) -> field.exists())
                       .toArray(BooleanExpression[]::new)));
     }

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryToPipelineTest.java

@@ -628,4 +628,64 @@ public void supportsOrOperator() throws Exception {
         firestore.pipeline().createFrom(query1).execute().get().getResults();
     verifyResults(snapshot, map("foo", 1L, "bar", 2L), map("foo", 3L, "bar", 10L));
   }
+
+  @Test
+  public void testNotEqualIncludesMissingField() throws Exception {
+    CollectionReference collRef =
+        testCollectionWithDocs(
+            ImmutableMap.of(
+                "1", map("foo", 1L, "bar", 1L),
+                "2", map("foo", 2L) // Missing "bar"
+                ));
+    Query query1 = collRef.whereNotEqualTo("bar", 1L);
+    List<PipelineResult> snapshot =
+        firestore.pipeline().createFrom(query1).execute().get().getResults();
+    // document "2" should be included because "bar" is missing, which is not equal to 1.
+    verifyResults(snapshot, map("foo", 2L));
+  }
+
+  @Test
+  public void testNotInIncludesMissingField() throws Exception {
+    CollectionReference collRef =
+        testCollectionWithDocs(
+            ImmutableMap.of(
+                "1", map("foo", 1L, "bar", 1L),
+                "2", map("foo", 2L) // Missing "bar"
+                ));
+    Query query1 = collRef.whereNotIn("bar", Arrays.asList(1L));
+    List<PipelineResult> snapshot =
+        firestore.pipeline().createFrom(query1).execute().get().getResults();
+    // document "2" should be included because "bar" is missing, which is not in [1].
+    verifyResults(snapshot, map("foo", 2L));
+  }
+
+  @Test
+  public void testInequalityMaintainsExistenceFilter() throws Exception {
+    CollectionReference collRef =
+        testCollectionWithDocs(
+            ImmutableMap.of(
+                "1", map("foo", 1L, "bar", 0L),
+                "2", map("foo", 2L) // Missing "bar"
+                ));
+    Query query1 = collRef.whereLessThan("bar", 1L);
+    List<PipelineResult> snapshot =
+        firestore.pipeline().createFrom(query1).execute().get().getResults();
+    // document "2" should be excluded because "bar" is missing.
+    verifyResults(snapshot, map("foo", 1L, "bar", 0L));
+  }
+
+  @Test
+  public void testExplicitOrderMaintainsExistenceFilter() throws Exception {
+    CollectionReference collRef =
+        testCollectionWithDocs(
+            ImmutableMap.of(
+                "1", map("foo", 1L, "bar", 1L),
+                "2", map("foo", 2L) // Missing "bar"
+                ));
+    Query query1 = collRef.orderBy("bar");
+    List<PipelineResult> snapshot =
+        firestore.pipeline().createFrom(query1).execute().get().getResults();
+    // document "2" should be excluded because "bar" is missing and we have explicit order.
+    verifyResults(snapshot, map("foo", 1L, "bar", 1L));
+  }
 }

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

@@ -1095,4 +1095,39 @@ private ListenResponse filter(int documentCount) {
     response.setFilter(ExistenceFilter.newBuilder().setCount(documentCount).build());
     return response.build();
   }
+
+  @Test
+  public void testInequalityIncludesAndSortsMissingFields() throws Exception {
+    setDocument("doc1", map("key", 1));
+    setDocument("doc2", map("key", 2));
+    setDocument("doc3", map("other", 1)); // missing "key"
+    setDocument("doc4", map("key", null));
+
+    final Query query = randomColl.whereNotEqualTo("key", 1);
+    QuerySnapshotEventListener listener =
+        QuerySnapshotEventListener.builder().setInitialEventCount(1).build();
+    ListenerRegistration registration = query.addSnapshotListener(listener);
+
+    try {
+      listener.eventsCountDownLatch.awaitInitialEvents();
+    } finally {
+      registration.remove();
+    }
+
+    ListenerAssertions listenerAssertions = listener.assertions();
+    listenerAssertions.noError();
+
+    if (getFirestoreEdition() == FirestoreEdition.ENTERPRISE) {
+      // Expect doc2, doc3, doc4. doc1 excluded.
+      // Order: Missing/Null (which are equal in sort) < Number.
+      // Missing/Null sorted by __name__.
+      // doc3 < doc4.
+      // So: doc3, doc4, doc2.
+      List<String> expectedOrder = Arrays.asList("doc3", "doc4", "doc2");
+      assertEquals(expectedOrder, listenerAssertions.addedIds);
+    } else {
+      List<String> expectedOrder = singletonList("doc2");
+      assertEquals(expectedOrder, listenerAssertions.addedIds);
+    }
+  }
 }