From 08d8d62d2cefab30711979e4ea69c6d470c696aa Mon Sep 17 00:00:00 2001 From: englefly Date: Sun, 16 Aug 2026 12:44:58 +0800 Subject: [PATCH] [fix](fe) Rewrite LogicalGenerate lateral conjuncts together with generators ### What problem does this PR solve? Problem Summary: GenerateExpressionRewrite rewrote only LogicalGenerate.getGenerators() and rebuilt the node via withGenerators(), which preserved the lateral ON conjuncts unchanged. Whole-tree ExprId replacements (e.g. any_value wrapping of a group-by key in EliminateGroupByKeyByUniform / EliminateGroupByKey) that renamed a slot referenced by an ON conjunct therefore left the conjunct with a stale ExprId after the child switched to the wrapped slot, and final slot validation rejected the query. Fix: GenerateExpressionRewrite now rewrites getConjuncts() in the same operation and rebuilds the node with a new LogicalGenerate.withGeneratorsAndConjuncts() helper, so generators and lateral ON conjuncts stay consistent under any expression rewrite. ### Release note None ### Check List (For Author) - Test: Unit Test - New GenerateConjunctRewriteTest.testExprIdRewriterRewritesLateralConjuncts: builds a LogicalGenerate with a conjunct referencing a slot, runs ExprIdRewriter with an old->new ExprId map and asserts the conjunct is rewritten (fails on the old code: expected 999 but was the old id). - Full GenerateConjunctRewriteTest / EliminateGroupByKeyByUniformTest / MergeGeneratesTest / FdTest classes are green. - Behavior changed: No. Internal correctness fix for plan rewriting; no intended plan-shape or performance change. - Does this need documentation: No --- .../rules/expression/ExpressionRewrite.java | 12 ++- .../trees/plans/logical/LogicalGenerate.java | 17 ++++ .../rewrite/GenerateConjunctRewriteTest.java | 79 +++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/GenerateConjunctRewriteTest.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRewrite.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRewrite.java index 2b77c7d927945b..bfcdd84a6ae6da 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRewrite.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRewrite.java @@ -126,10 +126,18 @@ public Rule build() { List newGenerators = generators.stream() .map(func -> (Function) rewriter.rewrite(func, context)) .collect(ImmutableList.toImmutableList()); - if (generators.equals(newGenerators)) { + // lateral ON conjuncts must be rewritten together with the generators: + // they reference the child output, so an ExprId replacement (e.g. any_value + // wrapping in EliminateGroupByKeyByUniform) that is not applied here would + // leave a stale slot and fail final slot validation. + List conjuncts = generate.getConjuncts(); + List newConjuncts = conjuncts.stream() + .map(conjunct -> rewriter.rewrite(conjunct, context)) + .collect(ImmutableList.toImmutableList()); + if (generators.equals(newGenerators) && conjuncts.equals(newConjuncts)) { return generate; } - return generate.withGenerators(newGenerators); + return generate.withGeneratorsAndConjuncts(newGenerators, newConjuncts); }).toRule(RuleType.REWRITE_GENERATE_EXPRESSION); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalGenerate.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalGenerate.java index a8d83b1d64137a..d4764518499ae0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalGenerate.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalGenerate.java @@ -142,6 +142,23 @@ public LogicalGenerate withGenerators(List generators) { new LogicalGenerate<>(generators, newGeneratorOutput, expandColumnAlias, conjuncts, child())); } + /** + * update generators and lateral conjuncts in one operation, so that expression rewrites + * (e.g. ExprId replacement) never leave a conjunct referencing a stale slot from the + * previous child output. + */ + public LogicalGenerate withGeneratorsAndConjuncts( + List generators, List conjuncts) { + Preconditions.checkArgument(generators.size() == generatorOutput.size()); + List newGeneratorOutput = Lists.newArrayList(); + for (int i = 0; i < generators.size(); i++) { + newGeneratorOutput.add(generatorOutput.get(i).withNullable(generators.get(i).nullable())); + } + return AbstractPlan.copyWithSameId(this, () -> + new LogicalGenerate<>(generators, newGeneratorOutput, expandColumnAlias, + Utils.fastToImmutableList(conjuncts), child())); + } + @Override public LogicalGenerate withGroupExpression(Optional groupExpression) { return AbstractPlan.copyWithSameId(this, () -> diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/GenerateConjunctRewriteTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/GenerateConjunctRewriteTest.java new file mode 100644 index 00000000000000..231d66d11ae339 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/GenerateConjunctRewriteTest.java @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.rules.rewrite; + +import org.apache.doris.nereids.CascadesContext; +import org.apache.doris.nereids.jobs.JobContext; +import org.apache.doris.nereids.properties.PhysicalProperties; +import org.apache.doris.nereids.trees.expressions.EqualTo; +import org.apache.doris.nereids.trees.expressions.ExprId; +import org.apache.doris.nereids.trees.expressions.Slot; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.StatementScopeIdGenerator; +import org.apache.doris.nereids.trees.expressions.functions.generator.Unnest; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.logical.LogicalGenerate; +import org.apache.doris.nereids.trees.plans.logical.LogicalOneRowRelation; +import org.apache.doris.nereids.types.IntegerType; +import org.apache.doris.nereids.types.VarcharType; +import org.apache.doris.nereids.util.MemoTestUtils; +import org.apache.doris.utframe.TestWithFeService; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Map; + +/** + * Tests that ExprId replacement (e.g. any_value wrapping in EliminateGroupByKeyByUniform / + * EliminateGroupByKey) is also applied to LogicalGenerate lateral ON conjuncts. + */ +class GenerateConjunctRewriteTest extends TestWithFeService { + + @Test + void testExprIdRewriterRewritesLateralConjuncts() { + // Review P1: GenerateExpressionRewrite rewrote only getGenerators(); lateral ON + // conjuncts kept the stale ExprId after the child switched to wrapped slots, so + // final slot validation rejected the query. The conjuncts must be rewritten + // together with the generators. + SlotReference k = new SlotReference("k", IntegerType.INSTANCE); + SlotReference a = new SlotReference("a", IntegerType.INSTANCE); + SlotReference x = new SlotReference("x", VarcharType.SYSTEM_DEFAULT); + Unnest generator = new Unnest(new IntegerLiteral(1)); + EqualTo conjunct = new EqualTo(k, a); + LogicalGenerate generate = new LogicalGenerate<>( + ImmutableList.of(generator), ImmutableList.of(x), ImmutableList.of(), + ImmutableList.of(conjunct), new LogicalOneRowRelation( + StatementScopeIdGenerator.newRelationId(), ImmutableList.of(a))); + + ExprId newK = new ExprId(999); + Map replaceMap = ImmutableMap.of(k.getExprId(), newK); + CascadesContext cascadesContext = MemoTestUtils.createCascadesContext(connectContext, generate); + ExprIdRewriter rewriter = new ExprIdRewriter(new ExprIdRewriter.ReplaceRule(replaceMap, true), + new JobContext(cascadesContext, PhysicalProperties.ANY)); + LogicalGenerate newGenerate = (LogicalGenerate) rewriter.rewriteExpr(generate, replaceMap); + + Assertions.assertEquals(1, newGenerate.getConjuncts().size()); + Slot newKSlot = (Slot) ((EqualTo) newGenerate.getConjuncts().get(0)).child(0); + Assertions.assertEquals(newK, newKSlot.getExprId(), + "lateral ON conjunct must be rewritten with the new ExprId"); + } +}