Skip to content

Commit 25f4ddf

Browse files
hatyohatyo
authored
NULL handling fixes in plan generator (#3506)
This commit implements two fixes to improve `null` literal handling in query processing. **Fix #3500: Implement null-safe equality constraints in plan generator** Updated the plan generator's literal equality constraint logic to properly handle `null` comparisons. The previous implementation used a simple `EQUALS` comparison, which incorrectly returned `null` when either operand was `null`, breaking equality constraints between identical literals when `null`s are involved. The new predicate implements proper null-safe equality semantics: `(left IS NULL AND right IS NULL) OR (left IS NOT NULL AND right IS NOT NULL AND left = right)`. While this predicate is more complex and may impact plan cache performance, it will be replaced with a simpler `IS NOT DISTINCT FROM` operator once issue #3504 is resolved. **Fix #3503: Prevent silent exclusion of null-bound OrderedLiterals** Removed legacy behavior that silently excluded `null`-bound `OrderedLiteral` objects from query processing. This undocumented behavior was preventing predicate simplification optimizations, resulting in suboptimal query plans. `null`-bound `OrderedLiteral` objects are now properly included in both the `Literals` collection and `EvaluationContext`, enabling correct dereferencing of `CoV` objects in query plans, and therefore, improved predicate simplification opportunities, resulting in more efficient physical query plans. **Issues resolved:** #3500, #3503
1 parent cf64ec3 commit 25f4ddf

File tree

12 files changed

+506
-16
lines changed

12 files changed

+506
-16
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -990,7 +990,10 @@ private enum UnaryPhysicalOperator {
990990
IS_NOT_NULL_EI(Comparisons.Type.NOT_NULL, Type.TypeCode.ENUM, Objects::nonNull),
991991

992992
IS_NULL_ID(Comparisons.Type.IS_NULL, Type.TypeCode.UUID, Objects::isNull),
993-
IS_NOT_NULL_ID(Comparisons.Type.NOT_NULL, Type.TypeCode.UUID, Objects::nonNull);
993+
IS_NOT_NULL_ID(Comparisons.Type.NOT_NULL, Type.TypeCode.UUID, Objects::nonNull),
994+
995+
IS_NULL_NT(Comparisons.Type.IS_NULL, Type.TypeCode.NULL, Objects::isNull),
996+
IS_NOT_NULL_NT(Comparisons.Type.NOT_NULL, Type.TypeCode.NULL, Objects::nonNull);
994997

995998
@Nonnull
996999
private static final Supplier<BiMap<UnaryPhysicalOperator, PUnaryPhysicalOperator>> protoEnumBiMapSupplier =

‎fdb-record-layer-core/src/main/proto/record_query_plan.proto

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,6 +1102,9 @@ message PUnaryRelOpValue {
11021102

11031103
IS_NULL_ID = 19;
11041104
IS_NOT_NULL_ID = 20;
1105+
1106+
IS_NULL_NT = 21;
1107+
IS_NOT_NULL_NT = 22;
11051108
}
11061109
optional PRelOpValue super = 1;
11071110
optional PUnaryPhysicalOperator operator = 2;

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Literals.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,16 @@
2727
import com.google.common.base.Suppliers;
2828
import com.google.common.base.Verify;
2929
import com.google.common.collect.ImmutableList;
30-
import com.google.common.collect.ImmutableMap;
3130
import com.google.common.collect.Lists;
3231
import com.google.common.collect.Multiset;
3332
import com.google.common.collect.TreeMultiset;
3433
import com.google.protobuf.ByteString;
3534

3635
import javax.annotation.Nonnull;
3736
import javax.annotation.Nullable;
37+
import java.util.Collections;
3838
import java.util.HashMap;
39+
import java.util.LinkedHashMap;
3940
import java.util.List;
4041
import java.util.Map;
4142
import java.util.Objects;
@@ -56,12 +57,13 @@ public class Literals {
5657

5758
private Literals(@Nonnull final List<OrderedLiteral> orderedLiterals) {
5859
this.orderedLiterals = ImmutableList.copyOf(orderedLiterals);
60+
// Using unmodifiableMap because it allows null values, which are valid here
61+
// and represent either null constants or null prepared parameters in queries.
5962
this.asMapSupplier = Suppliers.memoize(() ->
60-
this.orderedLiterals
61-
.stream()
62-
.filter(orderedLiteral -> orderedLiteral.getLiteralObject() != null)
63-
.collect(ImmutableMap.toImmutableMap(OrderedLiteral::getConstantId,
64-
OrderedLiteral::getLiteralObject)));
63+
Collections.unmodifiableMap(
64+
this.orderedLiterals
65+
.stream()
66+
.collect(LinkedHashMap::new, (m, v) -> m.put(v.getConstantId(), v.getLiteralObject()), LinkedHashMap::putAll)));
6567
}
6668

6769
@Nonnull

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/MutablePlanGenerationContext.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
import com.apple.foundationdb.record.query.expressions.Comparisons;
2828
import com.apple.foundationdb.record.query.plan.QueryPlanConstraint;
2929
import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
30+
import com.apple.foundationdb.record.query.plan.cascades.predicates.AndPredicate;
31+
import com.apple.foundationdb.record.query.plan.cascades.predicates.OrPredicate;
3032
import com.apple.foundationdb.record.query.plan.cascades.predicates.QueryPredicate;
3133
import com.apple.foundationdb.record.query.plan.cascades.predicates.ValuePredicate;
3234
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
@@ -164,8 +166,24 @@ private void addEqualityConstraint(@Nonnull String leftTokenId, @Nonnull String
164166
}
165167
final var leftCov = ConstantObjectValue.of(Quantifier.constant(), leftTokenId, type);
166168
final var rightCov = ConstantObjectValue.of(Quantifier.constant(), rightTokenId, type);
169+
170+
// we can replace the relatively complex predicate below with a much simpler one: (leftCov isNotDistinctFrom rightCov)
171+
// once https://github.com/FoundationDB/fdb-record-layer/issues/3504 is in, but this is ok for now.
172+
173+
// Term1: left != null AND right != null AND left = right
174+
final var leftIsNotNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
175+
final var rightIsNotNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
167176
final var equalityPredicate = new ValuePredicate(leftCov, new Comparisons.ValueComparison(Comparisons.Type.EQUALS, rightCov));
168-
equalityConstraints.add(equalityPredicate);
177+
final var notNullComparison = AndPredicate.and(ImmutableList.of(leftIsNotNull, rightIsNotNull, equalityPredicate));
178+
179+
// Term2: left = null AND right = null
180+
final var leftIsNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
181+
final var rightIsNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
182+
final var bothAreNullComparison = AndPredicate.and(leftIsNull, rightIsNull);
183+
184+
// Term1 OR Term2
185+
final var constraint = OrPredicate.or(notNullComparison, bothAreNullComparison);
186+
equalityConstraints.add(constraint);
169187
}
170188

171189

‎fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/LogAppenderRule.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
package com.apple.foundationdb.relational.recordlayer;
2222

23+
import com.apple.foundationdb.relational.util.Assert;
2324
import org.apache.logging.log4j.Level;
2425
import org.apache.logging.log4j.LogManager;
2526
import org.apache.logging.log4j.core.LogEvent;
@@ -119,10 +120,16 @@ public boolean lastMessageIsCacheHit() {
119120
}
120121

121122
public boolean lastMessageIsCacheMiss() {
123+
if (logAppender.getLogs().isEmpty()) {
124+
Assert.failUnchecked("attempt to peak logger's last message although the logger is empty (did you forget to add 'options (log query)' maybe?)");
125+
}
122126
return getLastLogEventMessage().contains("planCache=\"miss\"");
123127
}
124128

125129
public boolean lastMessageIsCacheSkip() {
130+
if (logAppender.getLogs().isEmpty()) {
131+
Assert.failUnchecked("attempt to peak logger's last message although the logger is empty (did you forget to add 'options (log query)' maybe?)");
132+
}
126133
return getLastLogEventMessage().contains("planCache=\"skip\"");
127134
}
128135
}

‎fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/LiteralsTests.java

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/*
2+
* LiteralsTests.java
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2015-2025 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
package com.apple.foundationdb.relational.recordlayer.query;
22+
23+
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
24+
import com.apple.foundationdb.relational.util.Assert;
25+
import com.google.common.collect.ImmutableList;
26+
import com.google.common.collect.ImmutableSet;
27+
import org.junit.jupiter.api.Assertions;
28+
import org.junit.jupiter.api.Test;
29+
30+
import javax.annotation.Nonnull;
31+
import java.util.ArrayList;
32+
import java.util.Collections;
33+
import java.util.HashMap;
34+
import java.util.Optional;
35+
import java.util.Random;
36+
import java.util.Set;
37+
38+
/**
39+
* Test suite for {@link Literals}. Currently, it only tests that no {@link OrderedLiteral}(s) are skipped when
40+
* building a {@link Literals} object.
41+
*/
42+
public class LiteralsTests {
43+
44+
@Nonnull
45+
private final Random random = new Random(42L);
46+
47+
@Test
48+
void nullLiteralsAreNotExcluded() {
49+
// TODO use RandomizedTestUtils.
50+
final var tokenIndices = generateUniqueIntegers(random, 0, 1000, ARGUMENTS_SIZE);
51+
final var nullLiteralsCount = 1 + random.nextInt(ARGUMENTS_SIZE - 1); // at least 1 null literal.
52+
final ArrayList<OrderedLiteral> orderedLiterals = new ArrayList<>(ARGUMENTS_SIZE);
53+
final var expectedNullOrderedLiterals = ImmutableList.<OrderedLiteral>builderWithExpectedSize(nullLiteralsCount);
54+
final var expectedNonNullOrderedLiterals = ImmutableList.<OrderedLiteral>builderWithExpectedSize(ARGUMENTS_SIZE - nullLiteralsCount);
55+
int nullLiteralsAdded = 0;
56+
for (final var tokenIndex : tokenIndices) {
57+
if (nullLiteralsAdded == nullLiteralsCount) {
58+
final var nonNullLiteral = nonNullLiteral(tokenIndex, random.nextInt() % 10000);
59+
orderedLiterals.add(nonNullLiteral);
60+
expectedNonNullOrderedLiterals.add(nonNullLiteral);
61+
} else {
62+
final var nullLiteral = nullLiteral(tokenIndex);
63+
orderedLiterals.add(nullLiteral(tokenIndex));
64+
expectedNullOrderedLiterals.add(nullLiteral);
65+
nullLiteralsAdded++;
66+
}
67+
}
68+
Collections.shuffle(orderedLiterals);
69+
70+
final var literalsBuilder = Literals.newBuilder();
71+
orderedLiterals.forEach(literalsBuilder::addLiteral);
72+
final var actualLiterals = literalsBuilder.build();
73+
final var actualAsMap = actualLiterals.asMap();
74+
75+
final HashMap<String, Object> expectedMap = new HashMap<>();
76+
expectedNullOrderedLiterals.build().forEach(l -> expectedMap.put(l.getConstantId(), l.getLiteralObject()));
77+
expectedNonNullOrderedLiterals.build().forEach(l -> expectedMap.put(l.getConstantId(), l.getLiteralObject()));
78+
79+
Assertions.assertEquals(expectedMap, actualAsMap);
80+
}
81+
82+
@Nonnull
83+
private static OrderedLiteral nonNullLiteral(int tokenIndex, int value) {
84+
return new OrderedLiteral(Type.primitiveType(Type.TypeCode.LONG), value, null, null, tokenIndex, Optional.empty());
85+
}
86+
87+
@Nonnull
88+
private static OrderedLiteral nullLiteral(int tokenIndex) {
89+
return new OrderedLiteral(Type.primitiveType(Type.TypeCode.NULL), null /*literal value*/, null, null, tokenIndex, Optional.empty());
90+
}
91+
92+
private static final int ARGUMENTS_SIZE = 100;
93+
94+
@Nonnull
95+
public static Set<Integer> generateUniqueIntegers(@Nonnull final Random random, int min, int max, int count) {
96+
Assert.thatUnchecked(0 <= min && min < max, "invalid range boundaries");
97+
Assert.thatUnchecked(count > 0 && count <= (max - min), "pick values' range is smaller than the desired number of unique values");
98+
return random.ints(min, max + 1)
99+
.distinct()
100+
.limit(count)
101+
.boxed()
102+
.collect(ImmutableSet.toImmutableSet());
103+
}
104+
}

‎fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/cache/ConstraintValidityTests.java

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.apple.foundationdb.record.query.plan.QueryPlanConstraint;
2828
import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
2929
import com.apple.foundationdb.record.query.plan.cascades.predicates.AndPredicate;
30+
import com.apple.foundationdb.record.query.plan.cascades.predicates.OrPredicate;
3031
import com.apple.foundationdb.record.query.plan.cascades.predicates.QueryPredicate;
3132
import com.apple.foundationdb.record.query.plan.cascades.predicates.ValuePredicate;
3233
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
@@ -46,6 +47,7 @@
4647
import com.apple.foundationdb.relational.recordlayer.query.QueryPlan;
4748
import com.apple.foundationdb.relational.utils.SimpleDatabaseRule;
4849
import com.apple.foundationdb.relational.utils.TestSchemas;
50+
import com.google.common.collect.ImmutableList;
4951
import com.google.common.collect.ImmutableSet;
5052
import com.google.common.testing.FakeTicker;
5153
import org.apache.commons.lang3.tuple.Pair;
@@ -200,10 +202,19 @@ private static QueryPredicate equalsConstraint(int tokenIndex, int value) {
200202

201203
@Nonnull
202204
private static QueryPredicate covsEqualsConstraints(int leftTokenIndex, int rightTokenIndex) {
203-
return new ValuePredicate(ConstantObjectValue.of(Quantifier.constant(),
204-
constantId(leftTokenIndex), Type.primitiveType(Type.TypeCode.INT)),
205-
new Comparisons.ValueComparison(Comparisons.Type.EQUALS, ConstantObjectValue.of(Quantifier.constant(),
206-
constantId(rightTokenIndex), Type.primitiveType(Type.TypeCode.INT))));
205+
final var leftCov = ConstantObjectValue.of(Quantifier.constant(), constantId(leftTokenIndex), Type.primitiveType(Type.TypeCode.INT));
206+
final var rightCov = ConstantObjectValue.of(Quantifier.constant(), constantId(rightTokenIndex), Type.primitiveType(Type.TypeCode.INT));
207+
208+
final var leftIsNotNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
209+
final var rightIsNotNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
210+
final var equalityPredicate = new ValuePredicate(leftCov, new Comparisons.ValueComparison(Comparisons.Type.EQUALS, rightCov));
211+
final var notNullComparison = AndPredicate.and(ImmutableList.of(leftIsNotNull, rightIsNotNull, equalityPredicate));
212+
213+
final var leftIsNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
214+
final var rightIsNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
215+
final var bothAreNullComparison = AndPredicate.and(leftIsNull, rightIsNull);
216+
217+
return OrPredicate.or(notNullComparison, bothAreNullComparison);
207218
}
208219

209220
@Nonnull

‎fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/cache/RelationalPlanCacheTests.java

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
import com.apple.foundationdb.record.query.expressions.Comparisons;
2727
import com.apple.foundationdb.record.query.plan.QueryPlanConstraint;
2828
import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
29+
import com.apple.foundationdb.record.query.plan.cascades.predicates.AndPredicate;
30+
import com.apple.foundationdb.record.query.plan.cascades.predicates.OrPredicate;
2931
import com.apple.foundationdb.record.query.plan.cascades.predicates.PredicateWithValueAndRanges;
3032
import com.apple.foundationdb.record.query.plan.cascades.predicates.QueryPredicate;
3133
import com.apple.foundationdb.record.query.plan.cascades.predicates.RangeConstraints;
@@ -178,10 +180,19 @@ private static QueryPredicate strEq(final int tokenIndex1, final int tokenIndex2
178180
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
179181
@Nonnull
180182
private static QueryPredicate strEq(final int tokenIndex1, final Optional<String> scope1, final int tokenIndex2, final Optional<String> scope2) {
181-
return new ValuePredicate(ConstantObjectValue.of(Quantifier.constant(),
182-
constantId(tokenIndex1, scope1), Type.primitiveType(Type.TypeCode.STRING)),
183-
new Comparisons.ValueComparison(Comparisons.Type.EQUALS, ConstantObjectValue.of(Quantifier.constant(),
184-
constantId(tokenIndex2, scope2), Type.primitiveType(Type.TypeCode.STRING))));
183+
final var leftCov = ConstantObjectValue.of(Quantifier.constant(), constantId(tokenIndex1, scope1), Type.primitiveType(Type.TypeCode.STRING));
184+
final var rightCov = ConstantObjectValue.of(Quantifier.constant(), constantId(tokenIndex2, scope2), Type.primitiveType(Type.TypeCode.STRING));
185+
186+
final var leftIsNotNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
187+
final var rightIsNotNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.NOT_NULL));
188+
final var equalityPredicate = new ValuePredicate(leftCov, new Comparisons.ValueComparison(Comparisons.Type.EQUALS, rightCov));
189+
final var notNullComparison = AndPredicate.and(ImmutableList.of(leftIsNotNull, rightIsNotNull, equalityPredicate));
190+
191+
final var leftIsNull = new ValuePredicate(leftCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
192+
final var rightIsNull = new ValuePredicate(rightCov, new Comparisons.NullComparison(Comparisons.Type.IS_NULL));
193+
final var bothAreNullComparison = AndPredicate.and(leftIsNull, rightIsNull);
194+
195+
return OrPredicate.or(notNullComparison, bothAreNullComparison);
185196
}
186197

187198
@Nonnull

‎yaml-tests/src/test/java/YamlIntegrationTests.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,4 +295,9 @@ public void literalTests(YamlTest.Runner runner) throws Exception {
295295
public void transactionalCallsTest(YamlTest.Runner runner) throws Exception {
296296
runner.runYamsql("transactions-tests.yamsql");
297297
}
298+
299+
@TestTemplate
300+
public void literalExtractionTests(YamlTest.Runner runner) throws Exception {
301+
runner.runYamsql("null-extraction-tests.yamsql");
302+
}
298303
}

0 commit comments

Comments
 (0)