Address nullability of null-on-empty quantifiers and nested fields (#3426)

This PR addresses two problems, both of which are necessary to resolve
problems encountered by
`FDBSimpleGraphQueryTest.testSubselectHasNullOnEmptyAndIsNullPredicate`
when trying to integrate the planner rewrite rules (see: #3401).

The first is that `Quantifier.forEachWithNullOnEmpty` quantifiers were
not taking into account the fact that they could return `null` in their
return type. That meant that predicate simplification rules based on
simplifying `IS (NOT) NULL` based on the field type could make incorrect
inferences about the result of such quantifiers. This updates the result
type method for that class so that we now take that into account.

The second is that if a type had non-nullable fields, but it was a
nested type that lived inside a nullable field, then we’d report that
field values on the child field were non-nullable. This is incorrect, as
if the whole struct is `null`, field accesses on the child can return
`null`. This could also result in the same rules making incorrect
inferences about the type. This addresses this by looking at the types
as we walk down field access paths, and then taking into account their
nullability when we return the field value's return type.

I also looked into a different approach, where we’d just pro-actively
mark all fields of a nullable type as nullable. This unfortunately has
cross version compatibility problems with this verify:


https://github.com/FoundationDB/fdb-record-layer/blob/296b4e5b1e1196c92dd78ceda54bc9f82b1dbe4d/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDefaultOnEmptyPlan.java#L79

Essentially, if the inner type has some (nested) non-nullable fields,
then the nullable on empty value will differ in more ways than just the
top-level type being nullable. This causes cross-version continuation
tests to fail. We could solve _that_ by loosening the assert to only
require that the types by the same (recursively) ignoring nullability,
and then constructing a new type that combines field nullabilities.

There are also problems with the fact that this erases the nullability
constraints entirely, which means that you wouldn’t be able to assert
that, say, either the whole parent struct is null _or_ all of its
non-null fields are not null.

Author: alecgrieser

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Quantifier.java

@@ -193,6 +193,15 @@ public boolean isNullOnEmpty() {
             return isNullOnEmpty;
         }
 
+        @Nonnull
+        @Override
+        public Type getFlowedObjectType() {
+            // If null on empty, then we may return null, so we need to update
+            // the nullability of our returned type to reflect that
+            Type baseType = super.getFlowedObjectType();
+            return baseType.overrideIfNullable(isNullOnEmpty);
+        }
+
         @Override
         @Nonnull
         public Builder<? extends Quantifier, ? extends Builder<?, ?>> toBuilder() {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/Type.java

@@ -216,6 +216,15 @@ default Type notNullable() {
     @Nonnull
     Type withNullability(boolean newIsNullable);
 
+    @Nonnull
+    default Type overrideIfNullable(boolean shouldBeNullable) {
+        if (shouldBeNullable && !isNullable()) {
+            return withNullability(true);
+        } else {
+            return this;
+        }
+    }
+
     /**
      * Safe-casts {@code this} into a {@link Array}.
      *
@@ -1906,6 +1915,9 @@ public boolean isNullable() {
         @Nonnull
         @Override
         public Record withNullability(final boolean newIsNullable) {
+            if (isNullable == newIsNullable) {
+                return this;
+            }
             return new Record(name, newIsNullable, fields);
         }
 
@@ -2436,6 +2448,20 @@ public Field withName(@Nonnull final String newName) {
                 }
             }
 
+            @Nonnull
+            public Field withNullability(boolean newNullability) {
+                if (getFieldType().isNullable() == newNullability) {
+                    return this;
+                }
+                var newFieldType = getFieldType().withNullability(newNullability);
+                return new Field(newFieldType, fieldNameOptional, fieldIndexOptional);
+            }
+
+            @Nonnull
+            public Field withOverriddenTypeIfNullable(boolean shouldBeNullable) {
+                return shouldBeNullable ? withNullability(true) : this;
+            }
+
             @Override
             public boolean equals(final Object o) {
                 if (o == null) {
@@ -2795,6 +2821,9 @@ public boolean isNullable() {
         @Nonnull
         @Override
         public Array withNullability(final boolean newIsNullable) {
+            if (newIsNullable == isNullable) {
+                return this;
+            }
             return new Array(newIsNullable, elementType);
         }
 

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/FieldValue.java

@@ -81,6 +81,8 @@ public class FieldValue extends AbstractValue implements ValueWithChild {
 
     @Nonnull
     private final Supplier<List<String>> fieldNamesSupplier;
+    @Nonnull
+    private final Supplier<Type> resultTypeSupplier;
 
     private FieldValue(@Nonnull final Value childValue, @Nonnull final FieldPath fieldPath) {
         this.childValue = childValue;
@@ -90,6 +92,7 @@ private FieldValue(@Nonnull final Value childValue, @Nonnull final FieldPath fie
                         .stream()
                         .map(maybe -> maybe.orElseThrow(() -> new RecordCoreException("field name should have been set")))
                         .collect(Collectors.toList()));
+        resultTypeSupplier = Suppliers.memoize(this::computeResultType);
     }
 
     @Nonnull
@@ -130,7 +133,14 @@ public Optional<String> getLastFieldName() {
     @Nonnull
     @Override
     public Type getResultType() {
-        return fieldPath.getLastFieldType();
+        return resultTypeSupplier.get();
+    }
+
+    @Nonnull
+    private Type computeResultType() {
+        Type lastFieldType = fieldPath.getLastFieldType();
+        boolean anyNullable = childValue.getResultType().isNullable() || fieldPath.areAnyFieldTypesNullable();
+        return lastFieldType.overrideIfNullable(anyNullable);
     }
 
     @Nonnull
@@ -451,6 +461,11 @@ public Type getLastFieldType() {
             return getFieldTypes().get(size() - 1);
         }
 
+        public boolean areAnyFieldTypesNullable() {
+            Preconditions.checkArgument(!isEmpty());
+            return getFieldTypes().stream().anyMatch(Type::isNullable);
+        }
+
         public int size() {
             return fieldAccessors.size();
         }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDefaultOnEmptyPlan.java

@@ -38,11 +38,12 @@
 import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
 import com.apple.foundationdb.record.query.plan.cascades.Reference;
 import com.apple.foundationdb.record.query.plan.cascades.explain.Attribute;
+import com.apple.foundationdb.record.query.plan.cascades.explain.ExplainPlanVisitor;
 import com.apple.foundationdb.record.query.plan.cascades.explain.NodeInfo;
 import com.apple.foundationdb.record.query.plan.cascades.explain.PlannerGraph;
 import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression;
 import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpressionWithChildren;
-import com.apple.foundationdb.record.query.plan.cascades.explain.ExplainPlanVisitor;
+import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
 import com.apple.foundationdb.record.query.plan.cascades.values.DerivedValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.Value;
 import com.apple.foundationdb.record.query.plan.cascades.values.translation.TranslationMap;
@@ -79,7 +80,26 @@ public RecordQueryDefaultOnEmptyPlan(@Nonnull Quantifier.Physical inner,
         Verify.verify(inner.getFlowedObjectType().nullable().equals(onEmptyResultValue.getResultType().nullable()));
         this.inner = inner;
         this.onEmptyResultValue = onEmptyResultValue;
-        this.resultValue = new DerivedValue(ImmutableList.of(inner.getFlowedObjectValue(), onEmptyResultValue), inner.getFlowedObjectType());
+        this.resultValue = new DerivedValue(ImmutableList.of(inner.getFlowedObjectValue(), onEmptyResultValue), chooseNullableType(inner.getFlowedObjectType(), onEmptyResultValue.getResultType()));
+    }
+
+    /**
+     * Choose the type that is nullable, if any. That is, if one type is nullable and the other
+     * one is not, return the nullable one. Otherwise (that is, the two types have the same nullability),
+     * choose {@code type1}.
+     *
+     * @param type1 the first type
+     * @param type2 the second type
+     * @return whichever of {@code type1} and {@code type2} are nullable
+     *   or {@code type1} if both are not nullable (or both are nullable)
+     */
+    @Nonnull
+    private static Type chooseNullableType(@Nonnull Type type1, @Nonnull Type type2) {
+        if (type1.isNullable()) {
+            return type1;
+        } else {
+            return type2.isNullable() ? type2 : type1;
+        }
     }
 
     @Nonnull

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBSimpleQueryGraphTest.java

@@ -64,6 +64,7 @@
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.params.ParameterizedTest;
 
@@ -217,6 +218,7 @@ void testSimplePlanGraphWithNullableArray() {
     }
 
     @DualPlannerTest(planner = DualPlannerTest.Planner.CASCADES)
+    @Disabled(value = "Null-on-empty quantifiers must be below selects until we address: https://github.com/FoundationDB/fdb-record-layer/issues/3431")
     void testPlanQueryOnRestNoWithNullOnEmpty() {
         CascadesPlanner cascadesPlanner = setUp();
         final var plan = planGraph(
@@ -250,6 +252,7 @@ void testPlanQueryOnRestNoWithNullOnEmpty() {
     }
 
     @DualPlannerTest(planner = DualPlannerTest.Planner.CASCADES)
+    @Disabled(value = "Null-on-empty quantifiers must be below selects until we address: https://github.com/FoundationDB/fdb-record-layer/issues/3431")
     void testPlanQueryOnNameWithNullOnEmpty() {
         CascadesPlanner cascadesPlanner = setUp();
         final var plan = planGraph(

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/ConstantFoldingTestUtils.java

@@ -32,8 +32,10 @@
 import com.apple.foundationdb.record.query.plan.cascades.predicates.simplification.ConstantFoldingRuleSet;
 import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
 import com.apple.foundationdb.record.query.plan.cascades.values.ConstantObjectValue;
+import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.NullValue;
+import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.PromoteValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.ThrowsValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.Value;
@@ -48,16 +50,24 @@
 import javax.annotation.Nullable;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Optional;
 
 public class ConstantFoldingTestUtils {
 
     private static int counter;
 
     @Nonnull
-    private static final Type.Record aType = Type.Record.fromFields(ImmutableList.of(
-            Type.Record.Field.of(Type.primitiveType(Type.TypeCode.INT), Optional.of("aInt")),
-            Type.Record.Field.of(Type.primitiveType(Type.TypeCode.STRING), Optional.of("bString"))));
+    public static final Type.Record lowerType = Type.Record.fromFields(false, List.of(
+            Type.Record.Field.of(Type.primitiveType(Type.TypeCode.STRING, false), Optional.of("a_non_null")),
+            Type.Record.Field.of(Type.primitiveType(Type.TypeCode.STRING, true), Optional.of("b_nullable"))
+    ));
+
+    @Nonnull
+    public static final Type.Record upperType = Type.Record.fromFields(false, List.of(
+            Type.Record.Field.of(lowerType.withNullability(false), Optional.of("a_non_null")),
+            Type.Record.Field.of(lowerType.withNullability(true), Optional.of("b_nullable"))
+    ));
 
     @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
     public static final class ValueWrapper {
@@ -90,6 +100,16 @@ public EvaluationContext getEvaluationContext() {
             return getEvaluationContextMaybe().get();
         }
 
+        @Nonnull
+        public EvaluationContext getEvaluationContextOrEmpty() {
+            return evaluationContext.orElse(EvaluationContext.EMPTY);
+        }
+
+        @Nonnull
+        public ValueWrapper withNewValue(@Nonnull Value value) {
+            return new ValueWrapper(value, evaluationContext);
+        }
+
         @Override
         public String toString() {
             if (value() instanceof ConstantObjectValue) {
@@ -236,6 +256,18 @@ public static ValueWrapper promoteToBoolean(@Nonnull ValueWrapper valueWrapper)
         return ValueWrapper.of(valueWrapper.getEvaluationContextMaybe(), promoteValue);
     }
 
+    @Nonnull
+    public static ValueWrapper qov(Type type) {
+        final QuantifiedObjectValue qov = QuantifiedObjectValue.of(Quantifier.current(), type);
+        return new ValueWrapper(qov, Optional.empty());
+    }
+
+    @Nonnull
+    public static ValueWrapper fieldValue(@Nonnull ValueWrapper baseValue, @Nonnull String fieldName) {
+        final FieldValue fieldValue = FieldValue.ofFieldNameAndFuseIfPossible(baseValue.value(), fieldName);
+        return baseValue.withNewValue(fieldValue);
+    }
+
     @Nonnull
     public static RangeConstraints buildSingletonRange(@Nonnull Comparisons.Type comparisonType) {
         return buildSingletonRange(comparisonType, null);