Skip to content

Avoid a case where the planner was erroneously dropping predicates #3965

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alecgrieser wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Avoid a case where the planner was erroneously dropping predicates #3965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ade72f
Keep track of residual comparisons when merging and then include them…
alecgrieser Mar 2, 2026
364d9d5
use PredicateWithValueAndRanges instead of AndPredicate
alecgrieser Mar 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions ...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ComparisonRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,6 @@

@Nonnull
public MergeResult merge(@Nonnull ComparisonRange comparisonRange) {
final ImmutableList.Builder<Comparisons.Comparison> residualPredicatesBuilder = ImmutableList.builder();
if (comparisonRange.isEmpty()) {
return MergeResult.of(this);
}
Expand All @@ -389,15 +388,16 @@
return MergeResult.of(comparisonRange);
}

if (isEquality()) {
if (comparisonRange.isEquality()) {
return merge(comparisonRange.getEqualityComparison());
}

Verify.verify(isInequality());
Verify.verify(comparisonRange.isInequality());
final List<Comparisons.Comparison> comparisons =
Objects.requireNonNull(comparisonRange.getInequalityComparisons());

ComparisonRange resultRange = this;
final ImmutableList.Builder<Comparisons.Comparison> residualPredicatesBuilder = ImmutableList.builder();
for (final Comparisons.Comparison comparison : comparisons) {
MergeResult mergeResult = resultRange.merge(comparison);
resultRange = mergeResult.getComparisonRange();
Expand Down Expand Up @@ -465,6 +465,9 @@
* Class to represent the outcome of a merge operation.
*/
public static class MergeResult {
@Nonnull
private static final MergeResult EMPTY = MergeResult.of(ComparisonRange.EMPTY, ImmutableList.of());

Check warning on line 469 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ComparisonRange.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

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

Use `java.util.List.of` instead
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=F5D5A429A41065E1A46736387E4DF568

@Nonnull
private final ComparisonRange comparisonRange;
@Nonnull
Expand All @@ -476,6 +479,23 @@
this.residualComparisons = ImmutableList.copyOf(residualComparison);
}

@Nonnull
public MergeResult merge(@Nonnull Comparisons.Comparison comparison) {
final MergeResult mergedRange = comparisonRange.merge(comparison);
if (residualComparisons.isEmpty()) {
return mergedRange;
} else if (mergedRange.getResidualComparisons().isEmpty()) {
return MergeResult.of(mergedRange.getComparisonRange(), residualComparisons);
} else {
final List<Comparisons.Comparison> combinedResiduals = ImmutableList.<Comparisons.Comparison>builderWithExpectedSize(residualComparisons.size() + mergedRange.getResidualComparisons().size())
.addAll(residualComparisons)
.addAll(mergedRange.getResidualComparisons())
.build();

return MergeResult.of(mergedRange.getComparisonRange(), combinedResiduals);
}
}

@Nonnull
public ComparisonRange getComparisonRange() {
return comparisonRange;
Expand All @@ -486,15 +506,23 @@
return residualComparisons;
}

@Nonnull
public static MergeResult empty() {
return MergeResult.EMPTY;
}

Check warning on line 512 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ComparisonRange.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ComparisonRange.java#L510-L512 

Method always returns the same value (MergeResult.EMPTY)
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=04C74C54A1DC7C9812A3C9C04D6FD2D8

@Nonnull
public static MergeResult of(@Nonnull final ComparisonRange comparisonRange) {
return of(comparisonRange, ImmutableList.of());
}

@Nonnull
public static MergeResult of(@Nonnull final ComparisonRange comparisonRange,
@Nonnull final Comparisons.Comparison residualComparison) {
return new MergeResult(comparisonRange, ImmutableList.of(residualComparison));
}

@Nonnull
public static MergeResult of(@Nonnull final ComparisonRange comparisonRange,
@Nonnull final List<Comparisons.Comparison> residualComparisons) {
return new MergeResult(comparisonRange, residualComparisons);
Expand Down
73 changes: 55 additions & 18 deletions ...apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.apple.foundationdb.record.query.expressions.Comparisons;
import com.apple.foundationdb.record.query.plan.QueryPlanConstraint;
import com.apple.foundationdb.record.query.plan.cascades.AliasMap;
import com.apple.foundationdb.record.query.plan.cascades.ComparisonRange;
import com.apple.foundationdb.record.query.plan.cascades.ConstrainedBoolean;
import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier;
import com.apple.foundationdb.record.query.plan.cascades.PredicateMultiMap.PredicateCompensationFunction;
Expand Down Expand Up @@ -327,7 +328,7 @@
}
final var matchPair = matchPairOptional.get();
final var comparisonCompensation = matchPair.getLeft();
final var constraint = matchPair.getRight();

Check warning on line 331 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java#L331 

Move the declaration of "constraint" closer to the code that uses it
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=9B45856BD2C6CB3690CFBFF869482B37

final var compensatedQueryPredicateOptional =
translateValueAndComparisonsMaybe(v -> Optional.of(comparisonCompensation.applyToValue(v)),
Expand All @@ -343,22 +344,40 @@
final var alias = ((WithAlias)candidatePredicateWithValuesAndRanges).getParameterAlias();
final var predicateMappingBuilder =
PredicateMapping.regularMappingBuilder(originalQueryPredicate, this, candidatePredicate)
.setPredicateCompensation((ignore, boundParameterPrefixMap, pullUp) -> {
if (boundParameterPrefixMap.containsKey(alias)) {
return PredicateCompensationFunction.noCompensationNeeded();
}
return computeCompensationFunctionForLeaf(pullUp);
})
.setParameterAlias(alias)
.setConstraint(constraint);

Verify.verify(isSargable() == compensatedQueryPredicate.isSargable());
final QueryPredicate residualPredicate;
if (compensatedQueryPredicate.isSargable()) {
predicateMappingBuilder.setParameterAlias(alias);
predicateMappingBuilder.setComparisonRange(
Iterables.getOnlyElement(compensatedQueryPredicate.getRanges())
.asComparisonRange());
final ComparisonRange.MergeResult mergeResult = Iterables.getOnlyElement(compensatedQueryPredicate.getRanges()).asMergedComparisonRange();
predicateMappingBuilder.setComparisonRange(mergeResult.getComparisonRange());
if (mergeResult.getResidualComparisons().isEmpty()) {
residualPredicate = null;
} else {
final RangeConstraints.Builder newRangeConstraintsBuilder = RangeConstraints.newBuilder();
boolean allComparisonsAdded = mergeResult.getResidualComparisons().stream().allMatch(newRangeConstraintsBuilder::addComparisonMaybe);
final Optional<RangeConstraints> newRangeConstraints = newRangeConstraintsBuilder.build();
if (allComparisonsAdded && newRangeConstraints.isPresent()) {
residualPredicate = new PredicateWithValueAndRanges(((PredicateWithValueAndRanges)candidatePredicate).getValue(), ImmutableSet.of(newRangeConstraints.get()));
} else {
residualPredicate = compensatedQueryPredicate;

Check warning on line 365 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java#L365 

[New] This method is quite long [0] and deeply nested in multiple places [1, 2, 3] which can make it hard to understand and maintain. Consider extracting helper methods or reducing the nesting by using early breaks or returns.

[0] https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=559B6BC201104BD8BC8FB5E129BDBC88
[1] https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=C1891839C60A4D37D0996DC948CD8B1C
[2] https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=1CA674251C41D9289B9E99BEC4451706
[3] https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=AD2D161DA4E938AE9C5899475F038270
}
}
} else {
residualPredicate = null;
}
predicateMappingBuilder.setPredicateCompensation(((partialMatch, boundParameterPrefixMap, pullUp) -> {
if (boundParameterPrefixMap.containsKey(alias)) {
if (residualPredicate == null) {
return PredicateCompensationFunction.noCompensationNeeded();
} else {
return residualPredicate.computeCompensationFunction(partialMatch, originalQueryPredicate, boundParameterPrefixMap, pullUp);
}
}
return computeCompensationFunctionForLeaf(pullUp);
}));
return Optional.of(predicateMappingBuilder.build());

Check warning on line 381 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java#L350-L381 

Clone with 2 instances of length 23
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=17D92EC829F0A02F095CB4AEED814FC6
} else {
return Optional.empty();
Expand All @@ -374,21 +393,39 @@
final var alias = ((WithAlias)candidatePredicateWithValuesAndRanges).getParameterAlias();
final var predicateMappingBuilder =
PredicateMapping.regularMappingBuilder(originalQueryPredicate, this, candidatePredicate)
.setPredicateCompensation((ignore, boundParameterPrefixMap, pullUp) -> {
if (boundParameterPrefixMap.containsKey(alias)) {
return PredicateCompensationFunction.noCompensationNeeded();
}
return computeCompensationFunctionForLeaf(pullUp);
})
.setConstraint(constraint.compose(captureConstraint(candidatePredicateWithValuesAndRanges)));
Verify.verify(isSargable() == compensatedQueryPredicate.isSargable());
final QueryPredicate residualPredicate;
if (compensatedQueryPredicate.isSargable()) {
predicateMappingBuilder.setParameterAlias(alias);
predicateMappingBuilder.setComparisonRange(
Iterables.getOnlyElement(compensatedQueryPredicate.getRanges())
.asComparisonRange());
final ComparisonRange.MergeResult mergeResult = Iterables.getOnlyElement(compensatedQueryPredicate.getRanges()).asMergedComparisonRange();
predicateMappingBuilder.setComparisonRange(mergeResult.getComparisonRange());
if (mergeResult.getResidualComparisons().isEmpty()) {
residualPredicate = null;
} else {
final RangeConstraints.Builder newRangeConstraintsBuilder = RangeConstraints.newBuilder();
boolean allComparisonsAdded = mergeResult.getResidualComparisons().stream().allMatch(newRangeConstraintsBuilder::addComparisonMaybe);
final Optional<RangeConstraints> newRangeConstraints = newRangeConstraintsBuilder.build();
if (allComparisonsAdded && newRangeConstraints.isPresent()) {
residualPredicate = new PredicateWithValueAndRanges(((PredicateWithValueAndRanges)candidatePredicate).getValue(), ImmutableSet.of(newRangeConstraints.get()));
} else {
residualPredicate = compensatedQueryPredicate;
}
}
} else {
residualPredicate = null;
}
predicateMappingBuilder.setPredicateCompensation(((partialMatch, boundParameterPrefixMap, pullUp) -> {
if (boundParameterPrefixMap.containsKey(alias)) {
if (residualPredicate == null) {
return PredicateCompensationFunction.noCompensationNeeded();
} else {
return residualPredicate.computeCompensationFunction(partialMatch, compensatedQueryPredicate, boundParameterPrefixMap, pullUp);
}
}
return computeCompensationFunctionForLeaf(pullUp);
}));
return Optional.of(predicateMappingBuilder.build());

Check warning on line 428 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/PredicateWithValueAndRanges.java#L397-L428 

Clone with 2 instances of length 23
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3965%2Falecgrieser%2F03950-keep-range-constraints%3AHEAD&id=77912F88F941E21EACD74731E4658C3F
} else {
return Optional.of(
PredicateMapping.regularMappingBuilder(originalQueryPredicate, this, candidatePredicate)
Expand Down
17 changes: 10 additions & 7 deletions ...n/java/com/apple/foundationdb/record/query/plan/cascades/predicates/RangeConstraints.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,18 +149,21 @@ public Set<Comparisons.Comparison> getDeferredRanges() {
}

/**
* Returns an equivalent {@link ComparisonRange}.
* Returns an equivalent {@link ComparisonRange} along with any residual comparisons that
* could not be pushed into the range. Values that cannot be combined into the single range
* will need to be compensated for by the caller.
* Note: This method is created for compatibility reasons.
*
* @return An equivalent {@link ComparisonRange}.
* @return An equivalent {@link ComparisonRange} along with a set of residual comparisons
* that require compensation
*/
@Nonnull
public ComparisonRange asComparisonRange() {
var resultRange = ComparisonRange.EMPTY;
for (final var comparison : getComparisons()) {
resultRange = resultRange.merge(comparison).getComparisonRange();
public ComparisonRange.MergeResult asMergedComparisonRange() {
ComparisonRange.MergeResult mergeResult = ComparisonRange.MergeResult.empty();
for (final Comparisons.Comparison comparison : getComparisons()) {
mergeResult = mergeResult.merge(comparison);
}
return resultRange;
return mergeResult;
}

@Nonnull
Expand Down
Loading
Loading