morrySnow commented on code in PR #66565: URL: https://github.com/apache/doris/pull/66565#discussion_r3734111001
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/merge/MergeUtils.java: ########## @@ -0,0 +1,58 @@ +// 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.trees.plans.commands.merge; + +import org.apache.doris.nereids.rules.exploration.join.JoinReorderContext; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.plans.JoinType; +import org.apache.doris.nereids.trees.plans.logical.LogicalJoin; +import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; + +import com.google.common.collect.ImmutableList; + +/** + * Shared plan-construction helpers for MERGE INTO, used by both the internal OLAP path + * ({@link MergeIntoCommand}) and the external path + * ({@link org.apache.doris.nereids.trees.plans.commands.ExternalRowLevelMergePlanBuilder}). + */ +public class MergeUtils { + + private MergeUtils() { + } + + /** + * Build the base join between merge target and source, with the target on the LEFT (probe) + * side. Doris builds the hash table on the right child, and the target side is structurally + * the wide one: it must carry every table column plus the row identity for the sink, while + * the source usually only carries join keys and new values. Keeping the target on the probe Review Comment: The Javadoc mentions that "Doris builds the hash table on the right child" — it would be helpful to also explicitly note that (which excludes but allows and ) is a co-motivator for this choice. This makes the rationale self-contained for future readers who might wonder why RIGHT_OUTER was chosen over LEFT_OUTER. ########## fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/merge/MergeIntoCommandTest.java: ########## @@ -85,13 +85,38 @@ source, new NullLiteral(), Assertions.assertEquals(1, logicalJoin.getOtherJoinConjuncts().size()); Expression onClause = logicalJoin.getOtherJoinConjuncts().get(0); Assertions.assertEquals(new NullLiteral(), onClause); - Assertions.assertEquals(JoinType.LEFT_OUTER_JOIN, logicalJoin.getJoinType()); - Assertions.assertEquals(source, logicalJoin.left()); - Assertions.assertInstanceOf(LogicalSubQueryAlias.class, logicalJoin.right()); - LogicalSubQueryAlias<?> alias = (LogicalSubQueryAlias<?>) logicalJoin.right(); + // without WHEN NOT MATCHED clauses unmatched source rows are not needed, so the join + // is INNER, and the target stays on the left (probe) side + Assertions.assertEquals(JoinType.INNER_JOIN, logicalJoin.getJoinType()); + Assertions.assertEquals(source, logicalJoin.right()); + Assertions.assertInstanceOf(LogicalSubQueryAlias.class, logicalJoin.left()); + LogicalSubQueryAlias<?> alias = (LogicalSubQueryAlias<?>) logicalJoin.left(); Assertions.assertEquals("alias", alias.getAlias()); } + @Test + public void testGenerateBasePlanWithNotMatchedClause() throws Exception { + LogicalPlan source = new LogicalEmptyRelation(new RelationId(1), ImmutableList.of()); + MergeIntoCommand command = new MergeIntoCommand( + ImmutableList.of("ctl", "db", "tbl"), Optional.of("alias"), Optional.empty(), + source, new NullLiteral(), + ImmutableList.of(), + ImmutableList.of(new MergeNotMatchedClause( + Optional.empty(), ImmutableList.of(), ImmutableList.of())) + ); + + Class<?> clazz = Class.forName("org.apache.doris.nereids.trees.plans.commands.merge.MergeIntoCommand"); + Method generateBasePlan = clazz.getDeclaredMethod("generateBasePlan"); + generateBasePlan.setAccessible(true); + LogicalPlan result = (LogicalPlan) generateBasePlan.invoke(command); + Assertions.assertInstanceOf(LogicalJoin.class, result); + LogicalJoin<?, ?> logicalJoin = (LogicalJoin<?, ?>) result; + // WHEN NOT MATCHED needs the unmatched source rows: source is the preserved right side Review Comment: The new test asserts the left child is a LogicalSubQueryAlias but does not verify the alias value. For consistency with `testGenerateBasePlanWithAlias` (which extracts the alias and asserts `.getAlias()` equals "alias"), consider adding the same assertion here. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/merge/MergeUtils.java: ########## @@ -0,0 +1,58 @@ +// 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.trees.plans.commands.merge; + +import org.apache.doris.nereids.rules.exploration.join.JoinReorderContext; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.plans.JoinType; +import org.apache.doris.nereids.trees.plans.logical.LogicalJoin; +import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; + +import com.google.common.collect.ImmutableList; + +/** + * Shared plan-construction helpers for MERGE INTO, used by both the internal OLAP path + * ({@link MergeIntoCommand}) and the external path + * ({@link org.apache.doris.nereids.trees.plans.commands.ExternalRowLevelMergePlanBuilder}). + */ +public class MergeUtils { + + private MergeUtils() { + } + + /** + * Build the base join between merge target and source, with the target on the LEFT (probe) + * side. Doris builds the hash table on the right child, and the target side is structurally + * the wide one: it must carry every table column plus the row identity for the sink, while + * the source usually only carries join keys and new values. Keeping the target on the probe Review Comment: The Javadoc mentions that "Doris builds the hash table on the right child" — it would be helpful to also explicitly note that `RuntimeFilterGenerator.DENIED_JOIN_TYPES` (which excludes `LEFT_OUTER_JOIN` but allows `RIGHT_OUTER_JOIN` and `INNER_JOIN`) is a co-motivator for this choice. This makes the rationale self-contained for future readers who might wonder why RIGHT_OUTER was chosen over LEFT_OUTER. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
