Skip to content

Add validation/verification to the planner to avoid alias ambiguities and unresolved aliases #3405

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add validation/verification to the planner to avoid alias ambiguities and unresolved aliases #3405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/Bindings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,20 @@

import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.planprotos.PParameterComparison.PBindingKind;
import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier;
import com.apple.foundationdb.record.util.pair.Pair;
import com.google.common.base.Suppliers;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Supplier;

/**
* A map of bound parameter values passed to query evaluation.
Expand Down Expand Up @@ -117,9 +122,13 @@ public static Internal fromProto(@Nonnull final PlanSerializationContext seriali

public static final Bindings EMPTY_BINDINGS = new Bindings();

@Nonnull
private Supplier<Set<CorrelationIdentifier>> boundCorrelationAliasesSupplier;

private Bindings(@Nullable Bindings parent) {
this.values = new HashMap<>();
this.parent = parent;
this.boundCorrelationAliasesSupplier = Suppliers.memoize(this::computeBoundCorrelationAliases);
}

public Bindings() {
Expand Down Expand Up @@ -165,6 +174,20 @@ public List<Map.Entry<String, Object>> asMappingList() {
return resultBuilder.build();
}

@Nonnull
public Set<CorrelationIdentifier> getBoundCorrelationAliases() {
return boundCorrelationAliasesSupplier.get();
}

@Nonnull
private Set<CorrelationIdentifier> computeBoundCorrelationAliases() {
return asMappingList()
.stream()
.filter(entry -> Bindings.Internal.CORRELATION.isOfType(entry.getKey()))
.map(entry -> CorrelationIdentifier.of(Bindings.Internal.CORRELATION.identifier(entry.getKey())))
.collect(ImmutableSet.toImmutableSet());
}

@Override
public String toString() {
return "Bindings(" + asMappingList() + ")";
Expand Down
5 changes: 5 additions & 0 deletions ...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,11 @@ private void planPartial(@Nonnull final Supplier<Reference> referenceSupplier,
@Nonnull final Function<Reference, PlanContext> contextCreatorFunction,
@Nonnull final EvaluationContext evaluationContext) {
this.currentRoot = referenceSupplier.get();

// run sanity check to make sure that all aliases handed in can be uniquely resolved
Debugger.sanityCheck(() ->
currentRoot.verifyCorrelationsRecursive(evaluationContext.getBindings().getBoundCorrelationAliases()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How expensive do you think it would be to run this check here at the beginning of planning? Running it with every new expression (outside of a sanity check) is probably too much, but it would probably be a good idea to verify at the start that the initial input is okay. Maybe there's a concern that we'd start failing someone's query that is technically illegal (or that we identify as illegal as a bug) which results in failures in production code. We may also need to switch this away from the Verify framework so that we generate an appropriate error code if we're using this validate user input

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could always run it, sure! If this triggers I would always view it as a bug answer the plan generator should have caught it, so I guess verify is fine.


this.planContext = contextCreatorFunction.apply(currentRoot);
this.evaluationContext = evaluationContext;

Expand Down
8 changes: 7 additions & 1 deletion ...ore/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesRuleCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public void yieldUnknownExpression(@Nonnull final RelationalExpression expressio
}

private void yieldExpression(@Nonnull final RelationalExpression expression, final boolean isFinal) {
verifyChildrenMemoized(expression);
validateNewExpression(expression);
if (isFinal) {
if (root.insertFinalExpression(expression)) {
newFinalExpressions.add(expression);
Expand All @@ -229,6 +229,12 @@ private void yieldExpression(@Nonnull final RelationalExpression expression, fin
}
}

protected void validateNewExpression(@Nonnull final RelationalExpression expression) {
Debugger.sanityCheck(() -> verifyChildrenMemoized(expression));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this to be part of a sanity check? The old code always validated that the children were memoized, so this is making things laxer, at least in the production configuration of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. My thinking was that this has never triggered and we possibly can use the cycles elsewhere.

Debugger.sanityCheck(() -> getRoot().verifyCorrelationsForNewExpression(traversal, expression,
getEvaluationContext()));
}

private void verifyChildrenMemoized(@Nonnull RelationalExpression expression) {
for (final var quantifier : expression.getQuantifiers()) {
final var rangesOver = quantifier.getRangesOver();
Expand Down
128 changes: 128 additions & 0 deletions ...layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package com.apple.foundationdb.record.query.plan.cascades;

import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.EvaluationContext;
import com.apple.foundationdb.record.RecordCoreException;
import com.apple.foundationdb.record.query.plan.HeuristicPlanner;
import com.apple.foundationdb.record.query.plan.cascades.debug.Debugger;
Expand All @@ -40,6 +41,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.errorprone.annotations.CanIgnoreReturnValue;

import javax.annotation.Nonnull;
Expand Down Expand Up @@ -708,6 +710,132 @@ public boolean addPartialMatchForCandidate(final MatchCandidate candidate, final
return partialMatchMap.put(candidate, partialMatch);
}

public void verifyCorrelationsForNewExpression(@Nonnull final Traversal traversal,
@Nonnull final RelationalExpression expression,
@Nonnull final EvaluationContext evaluationContext) {
final Set<CorrelationIdentifier> correlatedToWithoutChildren;
if (expression instanceof RelationalExpressionWithChildren) {
correlatedToWithoutChildren = ((RelationalExpressionWithChildren)expression).getCorrelatedToWithoutChildren();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this doesn't go down the children of this expression and validate those correlations as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That gets a little more tricky. In a lot of cases the expression that is yielded is the only thing that has changed. In those cases, this is exhaustive. If there is a memoization call that returns a new reference, and an expression in that new reference has a problem, we wouldn't catch it. Maybe I can call it when the exploration of that new reference starts. That however, would make the graph validation at beginning of planning superfluous.

} else {
correlatedToWithoutChildren = expression.getCorrelatedTo();
}

final var visibleThroughEvaluationContext = evaluationContext.getBindings().getBoundCorrelationAliases();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for things like constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are bunch of test cases written using incomplete graphs, and sometimes using temp tables. This seems to be the easier way out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant themselves live in a different namespace among the bindings, so this is for actual ___corr_XXX

final var locallyVisibleAliases = expression.getLocallyVisibleAliases();

final var currentResolvedCorrelatedToBuilder =
ImmutableSet.<CorrelationIdentifier>builder();
final var currentUnresolvedCorrelatedToBuilder =
ImmutableSet.<CorrelationIdentifier>builder();
for (final var unresolvedAlias : correlatedToWithoutChildren) {
if (visibleThroughEvaluationContext.contains(unresolvedAlias) ||
locallyVisibleAliases.contains(unresolvedAlias)) {
currentResolvedCorrelatedToBuilder.add(unresolvedAlias);
} else {
// still unresolved
currentUnresolvedCorrelatedToBuilder.add(unresolvedAlias);
}
}

final var currentUnresolvedCorrelatedTo =
currentUnresolvedCorrelatedToBuilder.build();
final var currentResolvedCorrelatedTo =
currentResolvedCorrelatedToBuilder.build();

final var parentRefPaths = traversal.getParentRefPaths(this);

if (parentRefPaths.isEmpty()) {
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " + currentUnresolvedCorrelatedTo);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " + currentUnresolvedCorrelatedTo);
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: %s", currentUnresolvedCorrelatedTo);

To prevent us from calculating the error message even if the verify succeeds

} else {
for (final var parentRefPath : parentRefPaths) {
final var parentReference = parentRefPath.getReference();
parentReference.verifyCorrelationsForNewExpressionRecursive(traversal, parentRefPath.getExpression(),
parentRefPath.getQuantifier(), currentUnresolvedCorrelatedTo, currentResolvedCorrelatedTo);
}
}
}

private void verifyCorrelationsForNewExpressionRecursive(@Nonnull final Traversal traversal,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like there's a fair amount of duplicated code between this method and verifyCorrelationsForNewExpression. Would it be possible to have the base case call the recursive case (potentially with like a null value for quantifier if need be)? I think the base case would still be responsible for constructing the set of unresolved correlation aliases, etc., but you then wouldn't need separate methods that both walked up the tree

@Nonnull final RelationalExpression expression,
@Nonnull final Quantifier quantifier,
@Nonnull final Set<CorrelationIdentifier> unresolvedCorrelatedTo,
@Nonnull final Set<CorrelationIdentifier> resolvedCorrelatedTo) {

final Set<CorrelationIdentifier> localVisibleAliases;
if (expression.canCorrelate()) {
final var allLocallyVisibleAliases = expression.getLocallyVisibleAliases();
Verify.verify(allLocallyVisibleAliases.contains(quantifier.getAlias()));
localVisibleAliases =
Sets.difference(allLocallyVisibleAliases, ImmutableSet.of(quantifier.getAlias()));
} else {
localVisibleAliases = ImmutableSet.of();
}

final var intersection = Sets.intersection(localVisibleAliases, resolvedCorrelatedTo);
Verify.verify(intersection.isEmpty(), "ambiguous aliases: " + intersection);
final var currentResolvedCorrelatedToBuilder =
ImmutableSet.<CorrelationIdentifier>builder();
currentResolvedCorrelatedToBuilder.addAll(resolvedCorrelatedTo);

final var currentUnresolvedCorrelatedToBuilder =
ImmutableSet.<CorrelationIdentifier>builder();
for (final var unresolvedAlias : unresolvedCorrelatedTo) {
if (localVisibleAliases.contains(unresolvedAlias)) {
currentResolvedCorrelatedToBuilder.add(unresolvedAlias);
} else {
// still unresolved
currentUnresolvedCorrelatedToBuilder.add(unresolvedAlias);
}
}

final var currentUnresolvedCorrelatedTo =
currentUnresolvedCorrelatedToBuilder.build();
final var currentResolvedCorrelatedTo =
currentResolvedCorrelatedToBuilder.build();

final var parentRefPaths = traversal.getParentRefPaths(this);

if (parentRefPaths.isEmpty()) {
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " +
Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: %s",

currentUnresolvedCorrelatedTo);
} else {
for (final var parentRefPath : parentRefPaths) {
final var parentReference = parentRefPath.getReference();
parentReference.verifyCorrelationsForNewExpressionRecursive(traversal,
parentRefPath.getExpression(), parentRefPath.getQuantifier(), currentUnresolvedCorrelatedTo,
currentResolvedCorrelatedTo);
}
}
}

public void verifyCorrelationsRecursive(@Nonnull final Set<CorrelationIdentifier> visibleAliases) {
for (final var expression : getAllMemberExpressions()) {
final var locallyVisibleAliases = expression.getLocallyVisibleAliases();
final var intersection = Sets.intersection(visibleAliases, locallyVisibleAliases);
Verify.verify(intersection.isEmpty(), "ambiguous aliases: ", intersection);

final var allVisibleAliases = Sets.union(visibleAliases, locallyVisibleAliases);
final Set<CorrelationIdentifier> correlatedToWithoutChildren;
if (expression instanceof RelationalExpressionWithChildren) {
correlatedToWithoutChildren = ((RelationalExpressionWithChildren)expression).getCorrelatedToWithoutChildren();
} else {
correlatedToWithoutChildren = expression.getCorrelatedTo();
}
final var difference = Sets.difference(correlatedToWithoutChildren, allVisibleAliases);
Verify.verify(difference.isEmpty(), "unresolved aliases: " + difference);

for (final var quantifier : expression.getQuantifiers()) {
final var lowerReference = quantifier.getRangesOver();
if (expression.canCorrelate()) {
lowerReference.verifyCorrelationsRecursive(Sets.difference(allVisibleAliases,
ImmutableSet.of(quantifier.getAlias())));
} else {
lowerReference.verifyCorrelationsRecursive(visibleAliases);
}
}
}
}

/**
* Method to render the graph rooted at this reference. This is needed for graph integration into IntelliJ as
* IntelliJ only ever evaluates selfish methods. Add this method as a custom renderer for the type
Expand Down
7 changes: 7 additions & 0 deletions ...m/apple/foundationdb/record/query/plan/cascades/expressions/RecursiveUnionExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ public boolean canCorrelate() {
return true;
}

@Nonnull
@Override
public Set<CorrelationIdentifier> getLocallyVisibleAliases() {
return ImmutableSet.of(tempTableInsertAlias, tempTableScanAlias, initialStateQuantifier.getAlias(),
recursiveStateQuantifier.getAlias());
}

@Nonnull
@Override
public Value getResultValue() {
Expand Down
5 changes: 5 additions & 0 deletions ...a/com/apple/foundationdb/record/query/plan/cascades/expressions/RelationalExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,11 @@ default PartiallyOrderedSet<CorrelationIdentifier> getCorrelationOrder() {
return PartiallyOrderedSet.empty();
}

@Nonnull
default Set<CorrelationIdentifier> getLocallyVisibleAliases() {
return Quantifiers.aliases(getQuantifiers());
}

boolean equalsWithoutChildren(@Nonnull RelationalExpression other,
@Nonnull AliasMap equivalences);

Expand Down
7 changes: 7 additions & 0 deletions ...ain/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementInJoinRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.apple.foundationdb.record.query.plan.cascades.matching.structure.CollectionMatcher;
import com.apple.foundationdb.record.query.plan.cascades.properties.OrderingProperty;
import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
import com.apple.foundationdb.record.query.plan.cascades.values.ParameterValue;
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.apple.foundationdb.record.query.plan.plans.InComparandSource;
Expand Down Expand Up @@ -383,6 +384,7 @@ private static Stream<List<OrderingPartWithSource>> suffixForRemainingExplodes(@
private static boolean isSupportedExplodeValue(@Nonnull final Value explodeValue) {
return explodeValue instanceof LiteralValue<?> ||
explodeValue instanceof QuantifiedObjectValue ||
explodeValue instanceof ParameterValue ||
explodeValue.isConstant();
}

Expand Down Expand Up @@ -417,6 +419,11 @@ private static InSource computeInSource(@Nonnull final Value explodeValue,
return attemptedSortOrder == null
? new InParameterSource(bindingName, alias)
: new SortedInParameterSource(bindingName, alias, attemptedSortOrder.isAnyDescending()); // TODO needs to distinguish between different descending orders
} else if (explodeValue instanceof ParameterValue) {
final var alias = ((ParameterValue)explodeValue).getBindingName();
return attemptedSortOrder == null
? new InParameterSource(bindingName, alias)
: new SortedInParameterSource(bindingName, alias, attemptedSortOrder.isAnyDescending()); // TODO needs to distinguish between different descending orders
} else if (explodeValue.isConstant()) {
return attemptedSortOrder == null
? new InComparandSource(bindingName, new Comparisons.ValueComparison(Comparisons.Type.IN, explodeValue))
Expand Down
4 changes: 4 additions & 0 deletions ...in/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementInUnionRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.apple.foundationdb.record.query.plan.cascades.matching.structure.CollectionMatcher;
import com.apple.foundationdb.record.query.plan.cascades.properties.OrderingProperty;
import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
import com.apple.foundationdb.record.query.plan.cascades.values.ParameterValue;
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.apple.foundationdb.record.query.plan.plans.InComparandSource;
Expand Down Expand Up @@ -146,6 +147,9 @@ public void onMatch(@Nonnull final ImplementationCascadesRuleCall call) {
} else if (explodeCollectionValue instanceof QuantifiedObjectValue) {
inSource = new InParameterSource(bindingName,
((QuantifiedObjectValue)explodeCollectionValue).getAlias().getId());
} else if (explodeCollectionValue instanceof ParameterValue) {
inSource = new InParameterSource(bindingName,
((ParameterValue)explodeCollectionValue).getBindingName());
} else if (explodeCollectionValue.isConstant()) {
inSource = new InComparandSource(bindingName,
new Comparisons.ValueComparison(Comparisons.Type.IN, explodeCollectionValue));
Expand Down
6 changes: 3 additions & 3 deletions ...va/com/apple/foundationdb/record/query/plan/cascades/rules/InComparisonToExplodeRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
import com.apple.foundationdb.record.RecordCoreException;
import com.apple.foundationdb.record.query.expressions.Comparisons;
import com.apple.foundationdb.record.query.plan.cascades.Column;
import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier;
import com.apple.foundationdb.record.query.plan.cascades.ExplorationCascadesRuleCall;
import com.apple.foundationdb.record.query.plan.cascades.ExplorationCascadesRule;
import com.apple.foundationdb.record.query.plan.cascades.ExplorationCascadesRuleCall;
import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
import com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression;
import com.apple.foundationdb.record.query.plan.cascades.expressions.SelectExpression;
Expand All @@ -39,6 +38,7 @@
import com.apple.foundationdb.record.query.plan.cascades.values.BooleanValue;
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
import com.apple.foundationdb.record.query.plan.cascades.values.ParameterValue;
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;
import com.apple.foundationdb.record.query.plan.cascades.values.RelOpValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
Expand Down Expand Up @@ -178,7 +178,7 @@ public void onMatch(@Nonnull final ExplorationCascadesRuleCall call) {
new Comparisons.ValueComparison(Comparisons.Type.EQUALS, QuantifiedObjectValue.of(newQuantifier.getAlias(), elementType))));

} else if (comparison instanceof Comparisons.ParameterComparison) {
explodeExpression = new ExplodeExpression(QuantifiedObjectValue.of(CorrelationIdentifier.of(((Comparisons.ParameterComparison)comparison).getParameter()), new Type.Array(elementType)));
explodeExpression = new ExplodeExpression(ParameterValue.of(((Comparisons.ParameterComparison)comparison).getParameter(), new Type.Array(elementType)));
newQuantifier = Quantifier.forEach(call.memoizeExploratoryExpression(explodeExpression));
transformedPredicates.add(new ValuePredicate(value,
new Comparisons.ValueComparison(Comparisons.Type.EQUALS, QuantifiedObjectValue.of(newQuantifier.getAlias(), elementType))));
Expand Down
Loading
Loading