Merge pull request #726 from MMcM/one-of-empty-null-option

Fixes #725: OneOfThemWith{Component,} does not distinguish empty vs null

docs/ReleaseNotes.md

@@ -42,7 +42,7 @@ The `FDBDatabase::getReadVersion()` method has been replaced with the `FDBRecord
 
 * **Bug fix** Fix 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 2 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
-* **Bug fix** Fix 3 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
+* **Bug fix** `Field.oneOfThem(boolean emptyIsUnknown)` for when the repeated field can otherwise distinguish null or null behavior is not desired [(Issue #725)](https://github.com/FoundationDB/fdb-record-layer/issues/725)
 * **Bug fix** Fix 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Performance** Improvement 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/BaseRepeatedField.java

@@ -29,15 +29,18 @@
 
 abstract class BaseRepeatedField extends BaseField {
 
-    public BaseRepeatedField(@Nonnull String fieldName) {
+    private final Field.OneOfThemEmptyMode emptyMode;
+
+    public BaseRepeatedField(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode) {
         super(fieldName);
+        this.emptyMode = emptyMode;
     }
 
     @Nullable
     @SuppressWarnings("unchecked")
     protected List<Object> getValues(@Nonnull MessageOrBuilder message) {
         final Descriptors.FieldDescriptor field = findFieldDescriptor(message);
-        if (message.getRepeatedFieldCount(field) == 0) {
+        if (emptyMode == Field.OneOfThemEmptyMode.EMPTY_UNKNOWN && message.getRepeatedFieldCount(field) == 0) {
             return null;
         } else {
             return (List<Object>) message.getField(field);
@@ -52,4 +55,29 @@ protected Descriptors.FieldDescriptor validateRepeatedField(@Nonnull Descriptors
         }
         return field;
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (!(o instanceof BaseRepeatedField)) {
+            return false;
+        }
+        if (!super.equals(o)) {
+            return false;
+        }
+        BaseRepeatedField baseRepeatedField = (BaseRepeatedField) o;
+        return emptyMode == baseRepeatedField.emptyMode;
+    }
+
+    @Override
+    public int hashCode() {
+        return super.hashCode() + emptyMode.hashCode();
+    }
+
+    @Override
+    public int planHash() {
+        return super.planHash() + emptyMode.ordinal();
+    }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/EmptyComparison.java

@@ -45,7 +45,7 @@ public class EmptyComparison extends BaseRepeatedField implements ComponentWithN
     private final boolean isEmpty;
 
     public EmptyComparison(@Nonnull String fieldName, boolean isEmpty) {
-        super(fieldName);
+        super(fieldName, Field.OneOfThemEmptyMode.EMPTY_NO_MATCHES);
         this.isEmpty = isEmpty;
     }
 

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Field.java

@@ -43,6 +43,17 @@ public Field(@Nonnull String fieldName) {
         this.fieldName = fieldName;
     }
 
+    /**
+     * How an empty / unset repeated field should be handled.
+     */
+    enum OneOfThemEmptyMode {
+        /** An empty repeated field causes {@code oneOfThem} predicates to return UNKNOWN, like a scalar NULL value. */
+        EMPTY_UNKNOWN,
+        /** An empty repeated field is treated like any other list, just with no elements, so none can match. */
+        EMPTY_NO_MATCHES
+        // TODO: A mode that depends on the nullability (versus field default value) of the record type's field in the descriptor / meta-data.
+    }
+
     /**
      * If the associated field is a submessage, this allows you to match against the fields within that submessage.
      * The child is evaluated and validated in the context of this field, not the record containing this field.
@@ -55,15 +66,27 @@ public QueryComponent matches(@Nonnull QueryComponent child) {
     }
 
     /**
-     * If the associated field is a repeated one, this allows you to match against one of those values, the record will
-     * be returned if any one of the values returns true. The record may be returned more than once.
-     * @return an OneOfThem that can have further assertions called on it about the value of the given field.
+     * If the associated field is a repeated one, this allows you to match against one of the repeated values.
+     * The record will be returned if any one of the values returns {@code true}. The same record may be returned more than once.
+     * If the repeated field is empty, the match result will be UNKNOWN.
+     * @return an OneOfThem that can have further assertions called on it about the value of the given field
      */
     @Nonnull
     public OneOfThem oneOfThem() {
         return new OneOfThem(fieldName);
     }
 
+    /**
+     * If the associated field is a repeated one, this allows you to match against one of the repeated values.
+     * The record will be returned if any one of the values returns {@code true}. The same record may be returned more than once.
+     * @param emptyMode whether an empty repeated field should cause an UNKNOWN result instead of failing to match any (and so returning FALSE)
+     * @return an OneOfThem that can have further assertions called on it about the value of the given field
+     */
+    @Nonnull
+    public OneOfThem oneOfThem(OneOfThemEmptyMode emptyMode) {
+        return new OneOfThem(fieldName, emptyMode);
+    }
+
     /**
      * Checks if the field has a value equal to the given comparand.
      * Evaluates to null if the field does not have a value.

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/OneOfThem.java

@@ -36,9 +36,15 @@
 public class OneOfThem {
     @Nonnull
     private final String fieldName;
+    private final Field.OneOfThemEmptyMode emptyMode;
 
     public OneOfThem(@Nonnull String fieldName) {
+        this(fieldName, Field.OneOfThemEmptyMode.EMPTY_UNKNOWN);
+    }
+
+    public OneOfThem(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode) {
         this.fieldName = fieldName;
+        this.emptyMode = emptyMode;
     }
 
     /**
@@ -49,7 +55,7 @@ public OneOfThem(@Nonnull String fieldName) {
      */
     @Nonnull
     public QueryComponent matches(@Nonnull QueryComponent child) {
-        return new OneOfThemWithComponent(fieldName, child);
+        return new OneOfThemWithComponent(fieldName, emptyMode, child);
     }
 
     /**
@@ -60,7 +66,7 @@ public QueryComponent matches(@Nonnull QueryComponent child) {
      */
     @Nonnull
     public QueryComponent equalsValue(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.EQUALS, comparand));
     }
 
@@ -72,7 +78,7 @@ public QueryComponent equalsValue(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent notEquals(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.NOT_EQUALS, comparand));
     }
 
@@ -84,7 +90,7 @@ public QueryComponent notEquals(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent greaterThan(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.GREATER_THAN, comparand));
     }
 
@@ -96,7 +102,7 @@ public QueryComponent greaterThan(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent greaterThanOrEquals(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.GREATER_THAN_OR_EQUALS, comparand));
     }
 
@@ -108,7 +114,7 @@ public QueryComponent greaterThanOrEquals(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent lessThan(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.LESS_THAN, comparand));
     }
 
@@ -120,7 +126,7 @@ public QueryComponent lessThan(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent lessThanOrEquals(@Nonnull Object comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.LESS_THAN_OR_EQUALS, comparand));
     }
 
@@ -131,7 +137,7 @@ public QueryComponent lessThanOrEquals(@Nonnull Object comparand) {
      */
     @Nonnull
     public QueryComponent startsWith(@Nonnull String comparand) {
-        return new OneOfThemWithComparison(fieldName,
+        return new OneOfThemWithComparison(fieldName, emptyMode,
                 new Comparisons.SimpleComparison(Comparisons.Type.STARTS_WITH, comparand));
     }
 
@@ -142,7 +148,8 @@ public QueryComponent startsWith(@Nonnull String comparand) {
      */
     @Nonnull
     public QueryComponent in(@Nonnull List<?> comparand) {
-        return new OneOfThemWithComparison(fieldName, new Comparisons.ListComparison(Comparisons.Type.IN, comparand));
+        return new OneOfThemWithComparison(fieldName, emptyMode,
+                new Comparisons.ListComparison(Comparisons.Type.IN, comparand));
     }
 
     /**
@@ -152,7 +159,8 @@ public QueryComponent in(@Nonnull List<?> comparand) {
      */
     @Nonnull
     public QueryComponent in(@Nonnull String param) {
-        return new OneOfThemWithComparison(fieldName, new Comparisons.ParameterComparison(Comparisons.Type.IN, param));
+        return new OneOfThemWithComparison(fieldName, emptyMode,
+                new Comparisons.ParameterComparison(Comparisons.Type.IN, param));
     }
 
     /**
@@ -167,7 +175,7 @@ public QueryComponent in(@Nonnull String param) {
      */
     @Nonnull
     public Text text() {
-        return new OneOfThemText(fieldName);
+        return new OneOfThemText(fieldName, emptyMode);
     }
 
     /**
@@ -182,7 +190,7 @@ public Text text() {
      */
     @Nonnull
     public Text text(@Nullable String tokenizerName) {
-        return new OneOfThemText(fieldName, tokenizerName);
+        return new OneOfThemText(fieldName, emptyMode, tokenizerName);
     }
 
     /**
@@ -202,6 +210,6 @@ public Text text(@Nullable String tokenizerName) {
      */
     @Nonnull
     public Text text(@Nullable String tokenizerName, @Nullable String defaultTokenizerName) {
-        return new OneOfThemText(fieldName, tokenizerName, defaultTokenizerName);
+        return new OneOfThemText(fieldName,  emptyMode, tokenizerName, defaultTokenizerName);
     }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/OneOfThemText.java

@@ -27,23 +27,25 @@
 class OneOfThemText extends Text {
     @Nonnull
     private final String fieldName;
+    private final Field.OneOfThemEmptyMode emptyMode;
 
-    OneOfThemText(@Nonnull String fieldName) {
-        this(fieldName, null);
+    OneOfThemText(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode) {
+        this(fieldName, emptyMode, null);
     }
 
-    OneOfThemText(@Nonnull String fieldName, @Nullable String tokenizerName) {
-        this(fieldName, tokenizerName, null);
+    OneOfThemText(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode, @Nullable String tokenizerName) {
+        this(fieldName, emptyMode, tokenizerName, null);
     }
 
-    OneOfThemText(@Nonnull String fieldName, @Nullable String tokenizerName, @Nullable String defaultTokenizerName) {
+    OneOfThemText(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode, @Nullable String tokenizerName, @Nullable String defaultTokenizerName) {
         super(tokenizerName, defaultTokenizerName);
         this.fieldName = fieldName;
+        this.emptyMode = emptyMode;
     }
 
     @Nonnull
     @Override
     ComponentWithComparison getComponent(@Nonnull Comparisons.Comparison comparison) {
-        return new OneOfThemWithComparison(fieldName, comparison);
+        return new OneOfThemWithComparison(fieldName, emptyMode, comparison);
     }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/OneOfThemWithComparison.java

@@ -46,7 +46,11 @@ public class OneOfThemWithComparison extends BaseRepeatedField implements Compon
     private final Comparisons.Comparison comparison;
 
     public OneOfThemWithComparison(@Nonnull String fieldName, @Nonnull Comparisons.Comparison comparison) {
-        super(fieldName);
+        this(fieldName, Field.OneOfThemEmptyMode.EMPTY_UNKNOWN, comparison);
+    }
+
+    public OneOfThemWithComparison(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode, @Nonnull Comparisons.Comparison comparison) {
+        super(fieldName, emptyMode);
         this.comparison = comparison;
     }
 

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/OneOfThemWithComponent.java

@@ -47,11 +47,19 @@ public class OneOfThemWithComponent extends BaseRepeatedField implements Compone
     private final ExpressionRef<QueryComponent> child;
 
     public OneOfThemWithComponent(@Nonnull String fieldName, @Nonnull QueryComponent child) {
-        this(fieldName, SingleExpressionRef.of(child));
+        this(fieldName, Field.OneOfThemEmptyMode.EMPTY_UNKNOWN, child);
     }
 
     public OneOfThemWithComponent(@Nonnull String fieldName, @Nonnull ExpressionRef<QueryComponent> child) {
-        super(fieldName);
+        this(fieldName, Field.OneOfThemEmptyMode.EMPTY_UNKNOWN, child);
+    }
+
+    public OneOfThemWithComponent(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode, @Nonnull QueryComponent child) {
+        this(fieldName, emptyMode, SingleExpressionRef.of(child));
+    }
+
+    public OneOfThemWithComponent(@Nonnull String fieldName, Field.OneOfThemEmptyMode emptyMode, @Nonnull ExpressionRef<QueryComponent> child) {
+        super(fieldName, emptyMode);
         this.child = child;
     }
 

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/expressions/QueryExpressionTest.java

@@ -40,6 +40,8 @@
 import com.google.protobuf.Message;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -140,13 +142,26 @@ public <M extends Message> Boolean evalMessage(@Nonnull FDBRecordStoreBase<M> st
         }
     };
 
-    @Test
-    public void testOneOfThemEqualsNoValues() throws Exception {
+    @ParameterizedTest(name = "testOneOfThemEqualsValue [emptyMode = {0}]")
+    @EnumSource(Field.OneOfThemEmptyMode.class)
+    public void testOneOfThemEqualsValue(Field.OneOfThemEmptyMode emptyMode) throws Exception {
+        final TestScalarFieldAccess oneRepeatedValue = TestScalarFieldAccess.newBuilder()
+                .addRepeatMe("fishes")
+                .build();
+        final QueryComponent component = field("repeat_me").oneOfThem(emptyMode).equalsValue("fishes");
+        component.validate(TestScalarFieldAccess.getDescriptor());
+        final Boolean eval = evaluate(component, oneRepeatedValue);
+        assertEquals(Boolean.TRUE, eval);
+    }
+
+    @ParameterizedTest(name = "testOneOfThemEqualsNoValues [emptyMode = {0}]")
+    @EnumSource(Field.OneOfThemEmptyMode.class)
+    public void testOneOfThemEqualsNoValues(Field.OneOfThemEmptyMode emptyMode) throws Exception {
         final TestScalarFieldAccess noRepeatedValues = TestScalarFieldAccess.newBuilder().build();
-        final QueryComponent component = field("repeat_me").oneOfThem().equalsValue("fishes");
+        final QueryComponent component = field("repeat_me").oneOfThem(emptyMode).equalsValue("fishes");
         component.validate(TestScalarFieldAccess.getDescriptor());
         final Boolean eval = evaluate(component, noRepeatedValues);
-        assertNull(eval);
+        assertEquals(emptyMode == Field.OneOfThemEmptyMode.EMPTY_UNKNOWN ? null : Boolean.FALSE, eval);
     }
 
     @Test