Skip to content

Commit

Permalink
Redo how sort keys are tracked by and-with-then planner.
Browse files Browse the repository at this point in the history 
* Equality comparisons take care of the corresponding key no matter where it occurs.
* Some unconditional sort advances got out-of-sync.
Fixes FoundationDB#1907: Sorting and comparing nested fields get confused
  • Loading branch information
@MMcM
MMcM committed Nov 17, 2022
1 parent ae45c52 commit 02a69bb
Showing 1 changed file with 42 additions and 57 deletions.
99 changes: 42 additions & 57 deletions ...layer-core/src/main/java/com/apple/foundationdb/record/query/plan/RecordQueryPlanner.java
View file
Expand Up @@ -106,11 +106,10 @@
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;
import java.util.function.Function;
import java.util.stream.Collectors;

/**
Expand Down Expand Up @@ -2009,15 +2008,10 @@ private class AndWithThenPlanner {
@Nonnull
private List<QueryComponent> unsatisfiedFilters;
/**
* If the sort is also a then, this iterates over its children.
* The set of sort keys that have not been satisfied (yet).
*/
@Nullable
private Iterator<KeyExpression> sortIterator;
/**
* The current sort child, or the sort itself, or {@code null} if the sort has been satisfied.
*/
@Nullable
private KeyExpression currentSort;
@Nonnull
private List<KeyExpression> unsatisfiedSorts;
/**
* True if the current child of the index {@link ThenKeyExpression Then} clause has a corresponding equality comparison in the filter.
*/
Expand Down Expand Up @@ -2070,6 +2064,7 @@ private AndWithThenPlanner(@Nonnull CandidateScan candidateScan,

unsatisfiedFilters = new ArrayList<>();
comparisons = new ScanComparisons.Builder();
unsatisfiedSorts = new ArrayList<>();
}

public ScoredPlan plan() {
Expand All @@ -2086,23 +2081,21 @@ public ScoredPlan plan() {
}
}
if (doneComparing) {
if (currentSort == null) {
if (unsatisfiedSorts.isEmpty()) {
if (!(candidateScan.index != null && candidateScan.index.isUnique() && childColumns >= candidateScan.index.getColumnSize())) {
// More index children than sorts, except for unique index sorted up far enough.
strictlySorted = false;
}
break;
}
// With inequalities or no filters, index ordering must match sort ordering.
if (currentSortMatches(child)) {
advanceCurrentSort();
} else {
if (!nextSortSatisfied(child)) {
break;
}
}
childColumns += child.getColumnSize();
}
if (currentSort != null) {
if (!unsatisfiedSorts.isEmpty()) {
return null;
}
if (comparisons.isEmpty()) {
Expand All @@ -2125,17 +2118,17 @@ public ScoredPlan plan() {
private void setupPlanState() {
unsatisfiedFilters = new ArrayList<>(filters);
comparisons = new ScanComparisons.Builder();
KeyExpression sortKey = sort;
if (sortKey instanceof GroupingKeyExpression) {
sortKey = ((GroupingKeyExpression) sortKey).getWholeKey();
}
if (sortKey instanceof ThenKeyExpression) {
ThenKeyExpression sortThen = (ThenKeyExpression) sortKey;
sortIterator = sortThen.getChildren().iterator();
currentSort = sortIterator.next();
} else {
currentSort = sortKey;
sortIterator = null;
if (sort != null) {
KeyExpression sortKey = sort;
if (sortKey instanceof GroupingKeyExpression) {
sortKey = ((GroupingKeyExpression) sortKey).getWholeKey();
}
if (sortKey instanceof ThenKeyExpression) {
ThenKeyExpression sortThen = (ThenKeyExpression) sortKey;
unsatisfiedSorts.addAll(sortThen.getChildren());
} else {
unsatisfiedSorts.add(sortKey);
}
}
}

Expand Down Expand Up @@ -2208,38 +2201,28 @@ private void planChild(@Nonnull KeyExpression child) {

private boolean planNestedFieldChild(@Nonnull KeyExpression child, @Nonnull NestedField filterField, @Nonnull QueryComponent filterChild) {
return planNestedFieldOrComponentChild(child, filterChild,
() -> planNestedField(candidateScan, child, filterField, null),
() -> planNestedField(candidateScan, child, filterField, currentSort));
(maybeSort) -> planNestedField(candidateScan, child, filterField, maybeSort));
}

private boolean planOneOfThemWithComponentChild(@Nonnull KeyExpression child, @Nonnull OneOfThemWithComponent oneOfThemWithComponent, @Nonnull QueryComponent filterChild) {
return planNestedFieldOrComponentChild(child, filterChild,
() -> planOneOfThemWithComponent(candidateScan, child, oneOfThemWithComponent, null),
() -> planOneOfThemWithComponent(candidateScan, child, oneOfThemWithComponent, currentSort));
(maybeSort) -> planOneOfThemWithComponent(candidateScan, child, oneOfThemWithComponent, maybeSort));
}

private boolean planNestedFieldOrComponentChild(@Nonnull KeyExpression child,
@Nonnull QueryComponent filterChild,
@Nonnull Supplier<ScoredPlan> unsortedPlanSupplier,
@Nonnull Supplier<ScoredPlan> sortedPlanSupplier) {
ScoredPlan scoredPlan = unsortedPlanSupplier.get();
@Nonnull Function<KeyExpression, ScoredPlan> maybeSortedPlan) {
ScoredPlan scoredPlan = maybeSortedPlan.apply(null);
ScanComparisons nextComparisons = getPlanComparisons(scoredPlan);
if (nextComparisons != null) {
if (!comparisons.isEquality() && nextComparisons.getEqualitySize() > 0) {
throw new Query.InvalidExpressionException(
"Two nested fields in the same and clause, combine them into one");
} else {
if (nextComparisons.isEquality()) {
// Equality comparisons might match required sort.
if (currentSortMatches(child)) {
advanceCurrentSort();
}
} else if (currentSort != null) {
if (!unsatisfiedSorts.isEmpty() && !nextComparisons.isEquality()) {
// Didn't plan to equality, need to try with sorting.
scoredPlan = sortedPlanSupplier.get();
if (scoredPlan != null) {
advanceCurrentSort();
}
scoredPlan = maybeSortedPlan.apply(unsatisfiedSorts.get(0));
nextComparisons = getPlanComparisons(scoredPlan);
}
if (scoredPlan != null) {
unsatisfiedFilters.remove(filterChild);
Expand All @@ -2248,6 +2231,7 @@ private boolean planNestedFieldOrComponentChild(@Nonnull KeyExpression child,
if (nextComparisons.isEquality()) {
foundComparison = true;
foundCompleteComparison = nextComparisons.getEqualitySize() == child.getColumnSize();
satisfyEqualitySort(child);
}
return true;
}
Expand All @@ -2256,21 +2240,24 @@ private boolean planNestedFieldOrComponentChild(@Nonnull KeyExpression child,
return false;
}

private boolean currentSortMatches(@Nonnull KeyExpression child) {
if (currentSort != null) {
if (currentSort.equals(child)) {
return true;
}
// A sort key corresponding to an equality comparison is (trivially) satisfied throughout.
@SuppressWarnings("PMD.EmptyWhileStmt")
private void satisfyEqualitySort(@Nonnull KeyExpression child) {
while (unsatisfiedSorts.remove(child)) {
// Keep removing all occurrences.
}
return false;
}

private void advanceCurrentSort() {
if (sortIterator != null && sortIterator.hasNext()) {
currentSort = sortIterator.next();
} else {
currentSort = null;
// Does this sort key from an inequality comparison or in the index after filters match what's pending?
private boolean nextSortSatisfied(@Nonnull KeyExpression child) {
if (unsatisfiedSorts.isEmpty()) {
return false;
}
if (child.equals(unsatisfiedSorts.get(0))) {
unsatisfiedSorts.remove(0);
return true;
}
return false;
}

private void planWithComparisonChild(@Nonnull KeyExpression child, @Nonnull FieldWithComparison field, @Nonnull QueryComponent filterChild) {
Expand Down Expand Up @@ -2344,9 +2331,7 @@ private void addedComparison(@Nonnull KeyExpression child, @Nonnull QueryCompone
unsatisfiedFilters.remove(filterChild);
if (foundComparison) {
foundCompleteComparison = true;
if (currentSortMatches(child)) {
advanceCurrentSort();
}
satisfyEqualitySort(child);
}
}

Expand Down

0 comments on commit 02a69bb

Please sign in to comment.