Skip to content

Commit

Permalink
Fixes #1907: Sorting and comparing nested fields get confused (#1918)
Browse files Browse the repository at this point in the history 
* Add some tests that cover the `advanceCurrentSort` calls from `planNestedFieldOrComponentChild`.
Each of these tests has two queries that should have identical plans.
The first one matches with both planners (by means of that code in the old case).
The second only currently passes with the Cascades planner (due to limitations of that same code).

* Redo how sort keys are tracked by and-with-then planner.
* Equality comparisons take care of the corresponding key no matter where it occurs.
* Some unconditional sort advances got out-of-sync.
Fixes #1907: Sorting and comparing nested fields get confused
  • Loading branch information
@MMcM
MMcM committed Nov 18, 2022
1 parent 52b44e4 commit 78c2837
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 59 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
97 changes: 97 additions & 0 deletions ...va/com/apple/foundationdb/record/provider/foundationdb/query/FDBNestedFieldQueryTest.java
View file
Expand Up @@ -762,6 +762,103 @@ void nestedWithBetween() throws Exception {
TestHelpers::assertDiscardedNone));
}

/**
* Verify that AND clauses in queries on nested record fields and sorting work properly.
*/
@DualPlannerTest
void nestedWithAndSorted() throws Exception {
final RecordMetaDataHook hook = metaData -> {
metaData.removeIndex("stats$school");
metaData.addIndex("RestaurantReviewer", "stats$cat_school_email",
concat(field("category"), field("stats").nest("start_date"), field("email")));
};
nestedWithAndSetup(hook);

RecordQuery query1 = RecordQuery.newBuilder()
.setRecordType("RestaurantReviewer")
.setFilter(Query.and(
Query.field("category").equalsValue(1),
Query.field("stats").matches(Query.field("start_date").greaterThan(0L))))
.setSort(field("stats").nest("start_date"))
.build();

// Index(stats$cat_school_email ([1, 0],[1]])
RecordQueryPlan plan1 = planner.plan(query1);
assertMatchesExactly(plan1,
indexPlan()
.where(indexName("stats$cat_school_email"))
.and(scanComparisons(range("([1, 0],[1]]"))));
assertEquals(-54634325, plan1.planHash(PlanHashable.PlanHashKind.LEGACY));
assertEquals(1388628388, plan1.planHash(PlanHashable.PlanHashKind.FOR_CONTINUATION));
assertEquals(1652535859, plan1.planHash(PlanHashable.PlanHashKind.STRUCTURAL_WITHOUT_LITERALS));
assertEquals(Collections.singletonList(2L), fetchResultValues(plan1, TestRecords4Proto.RestaurantReviewer.ID_FIELD_NUMBER,
ctx -> openNestedRecordStore(ctx, hook),
TestHelpers::assertDiscardedNone));

// Same query does more sorting than that.
RecordQuery query2 = RecordQuery.newBuilder()
.setRecordType("RestaurantReviewer")
.setFilter(Query.and(
Query.field("category").equalsValue(1),
Query.field("stats").matches(Query.field("start_date").greaterThan(0L))))
.setSort(concat(
field("stats").nest("start_date"),
field("email")))
.build();

RecordQueryPlan plan2 = planner.plan(query2);
assertEquals(plan1, plan2);
}

/**
* Verify that AND clauses in queries on nested record fields with nested equality and sorting work properly.
*/
@DualPlannerTest
void nestedWithAndSortedEquality() throws Exception {
final RecordMetaDataHook hook = metaData -> {
metaData.removeIndex("stats$school");
metaData.addIndex("RestaurantReviewer", "stats$cat_school_email",
concat(field("category"), field("stats").nest("start_date"), field("email")));
};
nestedWithAndSetup(hook);

RecordQuery query1 = RecordQuery.newBuilder()
.setRecordType("RestaurantReviewer")
.setFilter(Query.and(
Query.field("category").equalsValue(1),
Query.field("stats").matches(Query.field("start_date").equalsValue(1066L))))
.setSort(field("stats").nest("start_date"))
.build();

// Index(stats$cat_school_email [[1, 1066],[1, 1066]])
RecordQueryPlan plan1 = planner.plan(query1);
assertMatchesExactly(plan1,
indexPlan()
.where(indexName("stats$cat_school_email"))
.and(scanComparisons(range("[[1, 1066],[1, 1066]]"))));
assertEquals(-1814067790, plan1.planHash(PlanHashable.PlanHashKind.LEGACY));
assertEquals(-2078371741, plan1.planHash(PlanHashable.PlanHashKind.FOR_CONTINUATION));
assertEquals(319501900, plan1.planHash(PlanHashable.PlanHashKind.STRUCTURAL_WITHOUT_LITERALS));
assertEquals(Collections.singletonList(2L), fetchResultValues(plan1, TestRecords4Proto.RestaurantReviewer.ID_FIELD_NUMBER,
ctx -> openNestedRecordStore(ctx, hook),
TestHelpers::assertDiscardedNone));

// Some query with some reductive sorting.
RecordQuery query2 = RecordQuery.newBuilder()
.setRecordType("RestaurantReviewer")
.setFilter(Query.and(
Query.field("category").equalsValue(1),
Query.field("stats").matches(Query.field("start_date").equalsValue(1066L))))
.setSort(concat(
field("stats").nest("start_date"),
field("category"),
field("email")))
.build();

RecordQueryPlan plan2 = planner.plan(query2);
assertEquals(plan1, plan2);
}

/**
* Verify that queries on doubly nested records with fanout on the inner field work properly.
*/
Expand Down
9 changes: 7 additions & 2 deletions ...om/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreQueryTestBase.java
View file
Expand Up @@ -256,7 +256,10 @@ protected void nestedWithAndSetup(@Nullable RecordMetaDataHook hook) throws Exce
.setStartDate(0L)
.setSchoolName("University of Learning")
.setHometown("Home Town")
).build();

)
.setCategory(1)
.build();

TestRecords4Proto.RestaurantReviewer reviewer2 = TestRecords4Proto.RestaurantReviewer.newBuilder()
.setId(2L)
Expand All @@ -267,7 +270,9 @@ protected void nestedWithAndSetup(@Nullable RecordMetaDataHook hook) throws Exce
.setStartDate(1066L)
.setSchoolName("Human University")
.setHometown("Real Place")
).build();
)
.setCategory(1)
.build();

recordStore.saveRecord(reviewer1);
recordStore.saveRecord(reviewer2);
Expand Down
1 change: 1 addition & 0 deletions fdb-record-layer-core/src/test/proto/test_records_4.proto
View file
Expand Up @@ -31,6 +31,7 @@ message RestaurantReviewer {
required string name = 2 [(com.apple.foundationdb.record.field).index = {}];
optional string email = 3;
optional ReviewerStats stats = 4;
optional int32 category = 5;
}

message ReviewerStats {
Expand Down

0 comments on commit 78c2837

Please sign in to comment.