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

normen662 · 2025-06-18T20:04:10Z

This PR adds logic to

validate a query graph handed in to the CascadesPlanner
validate every expression yielded during REWRITING and PLANNING.

This validation logic is implemented in Reference.verifyCorrelationsRecursive and Reference.verifyCorrelationsForNewExpression.

I ran extensive verification tests with these checks enabled, but as coded up in the PR it will only be run if an insane Debugger is installed. That is currently the case for :fdb-record-layer-core:test but not for :fdb-relational-layer-core:test.

Upon running these tests the following problems surfaced:

There were quite a few tests using RecordQuery that caused InComparisonToExplodeRule to yield an incorrect ExplodeExpression of a QOV(parameter) where parameter is a bound parameter marker. Since QOV(...) is only properly defined over correlations, QOV(parameter) is an illegal alias reference as parameter is not a correlation. This was fixed by introducing ParameterValue that can dereference an actual parameter. Other rules such as ImplementInJoinRule and ImplementInUnionRule had to be adapted to create the proper InSource flavors.
RecursiveUnionExpression (and RecordQueryRecursiveUnionPlan) can correlate AND descendants can refer to the temp aliases. Logic was added to allow the alias resolution to understand that these temp table aliases are valid aliases to be used in subgraphs underneath RecursiveUnionExpression (and RecordQueryRecursiveUnionPlan).
We created illegal plans when QueryPlan.strictlySorted(...) was called (we incorrectly did not inherit the right quantifier alias). The fix is a one-liner in RecordQueryPredicatesFilterPlan.
Some test cases were written in a way that they handed only incomplete fragments to the planner. I sometimes changed the test case if that was easy. The more general fix is to allow an EvaluationContext to be passed in that the verification logic can use to identify additionally visible aliases.
Tons of adaptations in RuleTestHelper as
a. some query graphs were incomplete (dangling unresolved aliases)
b. the Traversal that is used inside of CascadesRuleCall to validate new expressions was incorrectly maintained in RuleTestHelper

… and unresolved aliases

alecgrieser · 2025-06-19T16:05:07Z

...r-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesRuleCall.java

@@ -229,6 +229,12 @@ private void yieldExpression(@Nonnull final RelationalExpression expression, fin
        }
    }

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


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

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

alecgrieser · 2025-06-19T16:07:36Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java

+                                                   @Nonnull final EvaluationContext evaluationContext) {
+        final Set<CorrelationIdentifier> correlatedToWithoutChildren;
+        if (expression instanceof RelationalExpressionWithChildren) {
+            correlatedToWithoutChildren = ((RelationalExpressionWithChildren)expression).getCorrelatedToWithoutChildren();


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

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.

alecgrieser · 2025-06-19T16:08:56Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java

+            correlatedToWithoutChildren = expression.getCorrelatedTo();
+        }
+
+        final var visibleThroughEvaluationContext = evaluationContext.getBindings().getBoundCorrelationAliases();


Is this for things like constants?

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

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

alecgrieser · 2025-06-19T16:10:19Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java

+        final var parentRefPaths = traversal.getParentRefPaths(this);
+
+        if (parentRefPaths.isEmpty()) {
+            Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " + currentUnresolvedCorrelatedTo);


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

alecgrieser · 2025-06-19T16:12:39Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java

+        final var parentRefPaths = traversal.getParentRefPaths(this);
+
+        if (parentRefPaths.isEmpty()) {
+            Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " +


Suggested change

Verify.verify(currentUnresolvedCorrelatedTo.isEmpty(), "unresolved aliases: " +

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

alecgrieser · 2025-06-19T16:41:55Z

...er-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

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


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

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.

alecgrieser · 2025-06-19T16:45:42Z

.../test/java/com/apple/foundationdb/record/provider/foundationdb/query/RecursiveUnionTest.java

@@ -321,9 +321,9 @@ private List<Long> multiplesOf(@Nonnull final List<Long> initial, long limit) th

            final var logicalPlan = Reference.initialOf(LogicalSortExpression.unsorted(Quantifier.forEach(Reference.initialOf(recursiveUnionPlan))));
            final var cascadesPlanner = (CascadesPlanner)planner;
-            final var plan = cascadesPlanner.planGraph(() -> logicalPlan, Optional.empty(), IndexQueryabilityFilter.TRUE, EvaluationContext.empty()).getPlan();
+            final var evaluationContext = putTempTableInContext(seedingTempTableAlias, seedingTempTable, null);
+            final var plan = cascadesPlanner.planGraph(() -> logicalPlan, Optional.empty(), IndexQueryabilityFilter.TRUE, evaluationContext).getPlan();


This was "wrong" before, but by calling cascadesPlanner::planGraph directly instead of FDBRecordStoreQueryTest::planGraph, it's not sending the plans through serialization verification

let me see if I can fix that.

.../test/java/com/apple/foundationdb/record/provider/foundationdb/query/RecursiveUnionTest.java

alecgrieser · 2025-06-19T16:49:26Z

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

@@ -245,6 +245,7 @@ public void bitmap(YamlTest.Runner runner) throws Exception {
    }

    @TestTemplate
+    @MaintainYamlTestConfig(YamlTestConfigFilters.CORRECT_EXPLAINS)


Suggested change

@MaintainYamlTestConfig(YamlTestConfigFilters.CORRECT_EXPLAINS)

Are there explains in the recursive-cte.yamsql test file that need to be modified?

Oh, I need to revert that. No, I sometimes do that as it doesn't run the multi server stuff if I repeatedly run it.

alecgrieser · 2025-06-19T16:55:38Z

...yer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/RuleTestHelper.java

        if (rule instanceof ImplementationCascadesRule) {
            for (RelationalExpression expression : expectedList) {
-                preExploreForRule(expression, true);
+                preExploreForRule(expression, Traversal.withRoot(Reference.initialOf(expression)),


Should this traversal be based on the root rather than the expression? It's possible the root and the expression can share subgraphs, so does that mean we'd need to copy the root and the expression as a unit to get the right traversal?

So, I assume that root includes group which directly contains expression. The rule under testing is run on group. So the pre-explore has to run on group.

add validation/verification to the planner to avoid alias ambiguities…

c4ba9cf

… and unresolved aliases

normen662 added the bug fix Change that fixes a bug label Jun 18, 2025

normen662 requested a review from alecgrieser June 18, 2025 20:29

alecgrieser changed the title ~~add validation/verification to the planner to avoid alias ambiguities…~~ Add validation/verification to the planner to avoid alias ambiguities and unresolved aliases Jun 19, 2025

alecgrieser requested changes Jun 19, 2025

View reviewed changes

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

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

normen662 commented Jun 18, 2025 • edited by alecgrieser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

normen662 commented Jun 18, 2025 •

edited by alecgrieser

Loading