Skip to content

Commit 331b53d

Browse files
alecgrieseralecgrieser
authored
Avoid Boolean simplification that would lead to expressions like x IS TRUE being simplified to FALSE (#3436)
The boolean value tests in #3401 were failing in this PRB: https://github.com/FoundationDB/fdb-record-layer/actions/runs/15878200321/attempts/1#summary-44771112943 I tracked this down, and the problem ended up being in some of the constant folding logic in `ConstantFoldingMultiConstraintPredicateRule`. Essentially, there was a check to see if two Boolean comparisons were of the same type (e.g., `TRUE` and `TRUE`), and if they were, then they'd be simplified to `ConstantPredicate.TRUE` or `ConstantPredicate.FALSE`. However, there was a problem with the logic in that it didn't handle the `NOT_NULL` and `UKNOWN` range types, which it would note as different from, say, `TRUE`, and then mark the whole predicate as `FALSE`. This would happen when given SQL predicates like `x IS TRUE` or `x IS FALSE`. The `IS` there is important, because it represents two range comparisons, one on `NOT_NULL` and the other on `IS TRUE/FALSE`. That meant that it would use `ConstantFoldingMultiConstraintPredicateRule` to simplify the predicate instead of `ConstantFoldingValuePredicateRule`. That other rule would use `ConstantFoldingUtil.foldComparisonMaybe`, which appears to have logic handling that case. To avoid this problem, I modified `ConstantFoldingMultiConstraintPredicateRule` so that it uses `ConstantFoldingUtil.foldComparisonMaybe` instead of its own logic. The rule then is now only responsible for `AND`ing the different comparison ranges together.
1 parent 8bff818 commit 331b53d

File tree

6 files changed

+199
-114
lines changed

6 files changed

+199
-114
lines changed

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/simplification/ConstantFoldingMultiConstraintPredicateRule.java

Lines changed: 15 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,12 @@
2626
import com.apple.foundationdb.record.query.plan.cascades.predicates.ConstantPredicate;
2727
import com.apple.foundationdb.record.query.plan.cascades.predicates.PredicateWithValueAndRanges;
2828
import com.apple.foundationdb.record.query.plan.cascades.predicates.RangeConstraints;
29-
import com.apple.foundationdb.record.query.plan.cascades.predicates.simplification.ConstantPredicateFoldingUtil.EffectiveConstant;
3029
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
3130
import com.google.common.base.Verify;
3231
import com.google.common.collect.ImmutableSet;
3332
import com.google.common.collect.Iterables;
3433

3534
import javax.annotation.Nonnull;
36-
3735
import java.util.Optional;
3836

3937
import static com.apple.foundationdb.record.query.plan.cascades.matching.structure.ListMatcher.exactly;
@@ -104,113 +102,45 @@ public void onMatch(@Nonnull final QueryPredicateSimplificationRuleCall call) {
104102

105103
@Nonnull
106104
private Optional<ConstantPredicate> foldConjunctionRangesMaybe(final Value lhs, @Nonnull final RangeConstraints rangeConstraints) {
107-
final var rangeCategoriesBuilder = ImmutableSet.<RangeCategory>builder();
108-
final var lhsOperand = EffectiveConstant.from(lhs);
109-
110105
// degenerate case, give up.
111106
if (rangeConstraints.getComparisons().isEmpty()) {
112107
return Optional.empty();
113108
}
114109

115-
for (final var comparison : rangeConstraints.getComparisons()) {
116-
if (comparison.getType().isUnary()) {
117-
if (comparison.getType() == Comparisons.Type.IS_NULL) {
118-
switch (lhsOperand) {
119-
case NULL:
120-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_TRUE);
121-
break;
122-
case TRUE: // fallthrough
123-
case FALSE: // fallthrough
124-
case NOT_NULL:
125-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_FALSE);
126-
break;
127-
default:
128-
break;
129-
}
130-
} else {
131-
rangeCategoriesBuilder.add(RangeCategory.UNKNOWN);
132-
}
133-
} else {
134-
if (!(comparison instanceof Comparisons.ValueComparison)) {
135-
rangeCategoriesBuilder.add(RangeCategory.UNKNOWN);
136-
continue;
137-
}
138-
final var valueComparison = (Comparisons.ValueComparison)comparison;
139-
final var rhsOperand = EffectiveConstant.from(valueComparison.getValue());
140-
switch (comparison.getType()) {
141-
case EQUALS:
142-
if (lhsOperand == EffectiveConstant.NULL || rhsOperand == EffectiveConstant.NULL) {
143-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_NULL);
144-
break;
145-
} else if (lhsOperand.equals(rhsOperand)) {
146-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_TRUE);
147-
break;
148-
} else {
149-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_FALSE);
150-
break;
151-
}
152-
case NOT_EQUALS:
153-
if (lhsOperand == EffectiveConstant.NULL || rhsOperand == EffectiveConstant.NULL) {
154-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_NULL);
155-
break;
156-
} else if (!lhsOperand.equals(rhsOperand)) {
157-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_TRUE);
158-
break;
159-
} else {
160-
rangeCategoriesBuilder.add(RangeCategory.SINGLETON_FALSE);
161-
break;
162-
}
163-
default:
164-
rangeCategoriesBuilder.add(RangeCategory.UNKNOWN);
165-
}
166-
}
110+
final var predicateCategoriesBuilder = ImmutableSet.<ConstantPredicateFoldingUtil.PredicateCategory>builder();
111+
for (Comparisons.Comparison comparison : rangeConstraints.getComparisons()) {
112+
predicateCategoriesBuilder.add(ConstantPredicateFoldingUtil.foldComparisonMaybe(lhs, comparison));
113+
114+
}
115+
final var predicateCategories = predicateCategoriesBuilder.build();
116+
117+
// if we have only one distinct range, then that's the one we go with
118+
if (predicateCategories.size() == 1) {
119+
return Iterables.getOnlyElement(predicateCategories).getPredicateMaybe();
167120
}
168-
// if we have more than one distinct range then it is impossible to find a single value that satisfies all of them
169-
final var rangeCategories = rangeCategoriesBuilder.build();
170121

171122
// if at least one range is FALSE, the returned result must be FALSE
172-
if (rangeCategories.contains(RangeCategory.SINGLETON_FALSE)) {
123+
if (predicateCategories.contains(ConstantPredicateFoldingUtil.PredicateCategory.SINGLETON_FALSE)) {
173124
return Optional.of(ConstantPredicate.FALSE);
174125
}
175126

176127
// if at least one range is UNKNOWN, give up.
177-
if (rangeCategories.contains(RangeCategory.UNKNOWN)) {
128+
if (predicateCategories.contains(ConstantPredicateFoldingUtil.PredicateCategory.UNKNOWN)) {
178129
return Optional.empty();
179130
}
180131

181-
if (rangeCategories.size() == 1) {
182-
final var singleRangeCategory = Iterables.getOnlyElement(rangeCategories);
183-
switch (singleRangeCategory) {
184-
case SINGLETON_TRUE:
185-
return Optional.of(ConstantPredicate.TRUE);
186-
case SINGLETON_FALSE:
187-
return Optional.of(ConstantPredicate.FALSE);
188-
case SINGLETON_NULL:
189-
return Optional.of(ConstantPredicate.NULL);
190-
default:
191-
return Optional.empty();
192-
}
193-
}
194-
195132
// we have multiple disjoint singleton ranges then it is impossible to find a single value satisfying all
196133
// of them ...
197134

198135
// ... unless we have a combination of TRUE and NULL then the result is NULL
199-
if (rangeCategories.stream().noneMatch(p -> p == RangeCategory.SINGLETON_FALSE)) {
136+
if (predicateCategories.stream().noneMatch(p -> p == ConstantPredicateFoldingUtil.PredicateCategory.SINGLETON_FALSE)) {
200137
// we must have TRUE and NULL
201-
Verify.verify(rangeCategories.contains(RangeCategory.SINGLETON_TRUE));
202-
Verify.verify(rangeCategories.contains(RangeCategory.SINGLETON_NULL));
138+
Verify.verify(predicateCategories.contains(ConstantPredicateFoldingUtil.PredicateCategory.SINGLETON_TRUE));
139+
Verify.verify(predicateCategories.contains(ConstantPredicateFoldingUtil.PredicateCategory.SINGLETON_NULL));
203140
return Optional.of(ConstantPredicate.NULL);
204141
}
205142

206143
// ... ok range is impossible, bailout.
207144
return Optional.of(ConstantPredicate.FALSE);
208145
}
209-
210-
private enum RangeCategory {
211-
SINGLETON_NULL,
212-
SINGLETON_TRUE,
213-
SINGLETON_FALSE,
214-
UNKNOWN
215-
}
216146
}

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/simplification/ConstantFoldingPredicateWithRangesRule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,6 @@ public void onMatch(@Nonnull final QueryPredicateSimplificationRuleCall call) {
9292
final var booleanSingletonRange = call.getBindings().get(booleanSingletonRangeMatcher);
9393
final var comparison = (Iterables.getOnlyElement(booleanSingletonRange.getComparisons()));
9494
final var lhsValue = root.getValue();
95-
ConstantPredicateFoldingUtil.foldComparisonMaybe(lhsValue, comparison).ifPresent(call::yieldResult);
95+
ConstantPredicateFoldingUtil.foldComparisonMaybe(lhsValue, comparison).getPredicateMaybe().ifPresent(call::yieldResult);
9696
}
9797
}

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/simplification/ConstantFoldingValuePredicateRule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,6 @@ public void onMatch(@Nonnull final QueryPredicateSimplificationRuleCall call) {
8383
final var root = call.getBindings().get(rootMatcher);
8484
final var comparison = call.getBindings().get(comparisonMatcher);
8585
final var lhsValue = root.getValue();
86-
ConstantPredicateFoldingUtil.foldComparisonMaybe(lhsValue, comparison).ifPresent(call::yieldResult);
86+
ConstantPredicateFoldingUtil.foldComparisonMaybe(lhsValue, comparison).getPredicateMaybe().ifPresent(call::yieldResult);
8787
}
8888
}

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/simplification/ConstantPredicateFoldingUtil.java

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,27 @@
3939
@API(API.Status.EXPERIMENTAL)
4040
public final class ConstantPredicateFoldingUtil {
4141

42+
public enum PredicateCategory {
43+
SINGLETON_NULL(ConstantPredicate.NULL),
44+
SINGLETON_TRUE(ConstantPredicate.TRUE),
45+
SINGLETON_FALSE(ConstantPredicate.FALSE),
46+
UNKNOWN(null),
47+
;
48+
49+
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
50+
@Nonnull
51+
private final Optional<ConstantPredicate> predicateMaybe;
52+
53+
PredicateCategory(@Nullable ConstantPredicate predicate) {
54+
this.predicateMaybe = Optional.ofNullable(predicate);
55+
}
56+
57+
@Nonnull
58+
Optional<ConstantPredicate> getPredicateMaybe() {
59+
return predicateMaybe;
60+
}
61+
}
62+
4263
/**
4364
* Analyzes the provided operand and comparison to produce a simplified, semantically equivalent {@link QueryPredicate}.
4465
*
@@ -47,36 +68,36 @@ public final class ConstantPredicateFoldingUtil {
4768
* @return An {@code Optional} containing the simplified {@link QueryPredicate} if both the operand and comparison
4869
* evaluate to constant values. Returns an empty {@code Optional} otherwise.
4970
*/
50-
public static Optional<QueryPredicate> foldComparisonMaybe(@Nonnull final Value operand,
51-
@Nonnull final Comparisons.Comparison comparison) {
71+
public static PredicateCategory foldComparisonMaybe(@Nonnull final Value operand,
72+
@Nonnull final Comparisons.Comparison comparison) {
5273
final var comparisonType = comparison.getType();
5374
final var lhsOperand = EffectiveConstant.from(operand);
5475
if (comparisonType.isUnary()) {
5576
switch (comparisonType) {
5677
case IS_NULL:
5778
switch (lhsOperand) {
5879
case NULL:
59-
return Optional.of(ConstantPredicate.TRUE);
80+
return PredicateCategory.SINGLETON_TRUE;
6081
case TRUE: // fallthrough
6182
case FALSE: // fallthrough
6283
case NOT_NULL:
63-
return Optional.of(ConstantPredicate.FALSE);
84+
return PredicateCategory.SINGLETON_FALSE;
6485
default:
65-
return Optional.empty();
86+
return PredicateCategory.UNKNOWN;
6687
}
6788
case NOT_NULL:
6889
switch (lhsOperand) {
6990
case NULL:
70-
return Optional.of(ConstantPredicate.FALSE);
91+
return PredicateCategory.SINGLETON_FALSE;
7192
case TRUE: // fallthrough
7293
case FALSE: // fallthrough
7394
case NOT_NULL:
74-
return Optional.of(ConstantPredicate.TRUE);
95+
return PredicateCategory.SINGLETON_TRUE;
7596
default:
76-
return Optional.empty();
97+
return PredicateCategory.UNKNOWN;
7798
}
7899
default:
79-
return Optional.empty();
100+
return PredicateCategory.UNKNOWN;
80101
}
81102
}
82103

@@ -91,39 +112,39 @@ public static Optional<QueryPredicate> foldComparisonMaybe(@Nonnull final Value
91112
final var rhsValue = simpleComparison.getComparand();
92113
rhsOperand = EffectiveConstant.from(rhsValue);
93114
} else {
94-
return Optional.empty();
115+
return PredicateCategory.UNKNOWN;
95116
}
96117

97118
switch (comparisonType) {
98119
case EQUALS: // fallthrough
99120
case NOT_EQUALS:
100121
if (lhsOperand == EffectiveConstant.NULL || rhsOperand == EffectiveConstant.NULL) {
101-
return Optional.of(ConstantPredicate.NULL);
122+
return PredicateCategory.SINGLETON_NULL;
102123
}
103124
break;
104125
default:
105-
return Optional.empty();
126+
return PredicateCategory.UNKNOWN;
106127
}
107128

108129
if (rhsOperand.isUnknownLiteral() || lhsOperand.isUnknownLiteral()) {
109-
return Optional.empty();
130+
return PredicateCategory.UNKNOWN;
110131
}
111132

112133
switch (comparisonType) {
113134
case EQUALS:
114135
if (lhsOperand.equals(rhsOperand)) {
115-
return Optional.of(ConstantPredicate.TRUE);
136+
return PredicateCategory.SINGLETON_TRUE;
116137
} else {
117-
return Optional.of(ConstantPredicate.FALSE);
138+
return PredicateCategory.SINGLETON_FALSE;
118139
}
119140
case NOT_EQUALS:
120141
if (!lhsOperand.equals(rhsOperand)) {
121-
return Optional.of(ConstantPredicate.TRUE);
142+
return PredicateCategory.SINGLETON_TRUE;
122143
} else {
123-
return Optional.of(ConstantPredicate.FALSE);
144+
return PredicateCategory.SINGLETON_FALSE;
124145
}
125146
default:
126-
return Optional.empty();
147+
return PredicateCategory.UNKNOWN;
127148
}
128149
}
129150

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

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@
3535
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
3636
import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
3737
import com.apple.foundationdb.record.query.plan.cascades.values.NullValue;
38-
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
3938
import com.apple.foundationdb.record.query.plan.cascades.values.PromoteValue;
39+
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
4040
import com.apple.foundationdb.record.query.plan.cascades.values.ThrowsValue;
4141
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
4242
import com.apple.foundationdb.record.query.plan.cascades.values.VariadicFunctionValue;
@@ -49,10 +49,14 @@
4949
import javax.annotation.Nonnull;
5050
import javax.annotation.Nullable;
5151
import java.util.Arrays;
52+
import java.util.Collection;
53+
import java.util.Collections;
5254
import java.util.HashMap;
5355
import java.util.List;
5456
import java.util.Optional;
5557

58+
import static org.junit.jupiter.api.Assertions.fail;
59+
5660
public class ConstantFoldingTestUtils {
5761

5862
private static int counter;
@@ -220,6 +224,16 @@ public static ValueWrapper covTrue() {
220224
return newCov(Type.primitiveType(Type.TypeCode.BOOLEAN), true);
221225
}
222226

227+
@Nonnull
228+
public static ValueWrapper nonNullBoolean() {
229+
return qov(Type.primitiveType(Type.TypeCode.BOOLEAN, false));
230+
}
231+
232+
@Nonnull
233+
public static ValueWrapper nullableBoolean() {
234+
return qov(Type.primitiveType(Type.TypeCode.BOOLEAN, true));
235+
}
236+
223237
@Nonnull
224238
public static ValueWrapper litString(@Nonnull final String value) {
225239
return ValueWrapper.of(LiteralValue.ofScalar(value));
@@ -275,18 +289,28 @@ public static RangeConstraints buildSingletonRange(@Nonnull Comparisons.Type com
275289

276290
@Nonnull
277291
public static RangeConstraints buildSingletonRange(@Nonnull Comparisons.Type comparisonType, @Nullable final Value comparand) {
278-
final var singletonRangeBuilder = RangeConstraints.newBuilder();
292+
Comparisons.Comparison comparison;
279293
switch (comparisonType) {
280294
case IS_NULL: // fallthrough
281295
case NOT_NULL:
282-
singletonRangeBuilder.addComparisonMaybe(new Comparisons.NullComparison(comparisonType));
296+
comparison = new Comparisons.NullComparison(comparisonType);
283297
break;
284298
default:
285-
singletonRangeBuilder.addComparisonMaybe(new Comparisons.ValueComparison(comparisonType, Verify.verifyNotNull(comparand)));
299+
comparison = new Comparisons.ValueComparison(comparisonType, Verify.verifyNotNull(comparand));
300+
}
301+
return buildMultiRange(Collections.singleton(comparison));
302+
}
303+
304+
@Nonnull
305+
public static RangeConstraints buildMultiRange(@Nonnull Collection<Comparisons.Comparison> comparisons) {
306+
var constraintsBuilder = RangeConstraints.newBuilder();
307+
for (Comparisons.Comparison comparison : comparisons) {
308+
Assertions.assertThat(constraintsBuilder.addComparisonMaybe(comparison))
309+
.as("should be able to add comparison: %s", comparison)
310+
.isTrue();
286311
}
287-
final var singletonRange = singletonRangeBuilder.build();
288-
Assertions.assertThat(singletonRange).isPresent();
289-
return singletonRange.get();
312+
return constraintsBuilder.build()
313+
.orElseGet(() -> fail("unable to construct range constraints over: " + comparisons));
290314
}
291315

292316
@Nonnull
@@ -307,7 +331,6 @@ public static QueryPredicate isNull(@Nonnull final Value value) {
307331
@Nonnull
308332
public static QueryPredicate isNullAsRange(@Nonnull final Value value) {
309333
return PredicateWithValueAndRanges.ofRanges(value, ImmutableSet.of(buildSingletonRange(Comparisons.Type.IS_NULL)));
310-
311334
}
312335

313336
@Nonnull
@@ -318,7 +341,15 @@ public static QueryPredicate areEqual(@Nonnull final Value value1, @Nonnull fina
318341
@Nonnull
319342
public static QueryPredicate areEqualAsRange(@Nonnull final Value value1, @Nonnull final Value value2) {
320343
return PredicateWithValueAndRanges.ofRanges(value1, ImmutableSet.of(buildSingletonRange(Comparisons.Type.EQUALS, value2)));
344+
}
321345

346+
@Nonnull
347+
public static QueryPredicate areNotNullAndEqualAsRange(@Nonnull final Value value1, @Nonnull final Value value2) {
348+
RangeConstraints multiRange = buildMultiRange(ImmutableSet.of(
349+
new Comparisons.NullComparison(Comparisons.Type.NOT_NULL),
350+
new Comparisons.ValueComparison(Comparisons.Type.EQUALS, value2)
351+
));
352+
return PredicateWithValueAndRanges.ofRanges(value1, ImmutableSet.of(multiRange));
322353
}
323354

324355
@Nonnull

0 commit comments

Comments
 (0)