From 91223a9a349fbc30026dd721246f29d4f8ef23e7 Mon Sep 17 00:00:00 2001 From: englefly Date: Thu, 22 Jan 2026 17:33:41 +0800 Subject: [PATCH] fix Expression.hasUnbound() --- .../nereids/trees/expressions/Expression.java | 51 +--- .../expressions/ExpressionUnboundTest.java | 233 ++++++++++++++++++ 2 files changed, 237 insertions(+), 47 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/ExpressionUnboundTest.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java index 0bf6ff950136e0..cd9aba84cfe126 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java @@ -52,6 +52,7 @@ import com.google.common.collect.Lists; import org.apache.commons.lang3.StringUtils; +import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.Set; @@ -87,53 +88,7 @@ public abstract class Expression extends AbstractTreeNode implements private final Supplier hashCodeCache = LazyCompute.of(this::computeHashCode); protected Expression(Expression... children) { - super(children); - - boolean hasUnbound = false; - switch (children.length) { - case 0: - this.depth = 1; - this.width = 1; - this.compareWidthAndDepth = supportCompareWidthAndDepth(); - this.fastChildrenHashCode = 0; - break; - case 1: - Expression child = children[0]; - this.depth = child.depth + 1; - this.width = child.width; - this.compareWidthAndDepth = child.compareWidthAndDepth && supportCompareWidthAndDepth(); - this.fastChildrenHashCode = child.fastChildrenHashCode() + 1; - break; - case 2: - Expression left = children[0]; - Expression right = children[1]; - this.depth = Math.max(left.depth, right.depth) + 1; - this.width = left.width + right.width; - this.compareWidthAndDepth = - left.compareWidthAndDepth && right.compareWidthAndDepth && supportCompareWidthAndDepth(); - this.fastChildrenHashCode = left.fastChildrenHashCode() + right.fastChildrenHashCode() + 2; - break; - default: - int maxChildDepth = 0; - int sumChildWidth = 0; - boolean compareWidthAndDepth = true; - int fastChildrenHashCode = 0; - for (Expression expression : children) { - child = expression; - maxChildDepth = Math.max(child.depth, maxChildDepth); - sumChildWidth += child.width; - hasUnbound |= child.hasUnbound; - compareWidthAndDepth &= child.compareWidthAndDepth; - fastChildrenHashCode = fastChildrenHashCode + expression.fastChildrenHashCode() + 1; - } - this.depth = maxChildDepth + 1; - this.width = sumChildWidth; - this.compareWidthAndDepth = compareWidthAndDepth; - this.fastChildrenHashCode = fastChildrenHashCode; - } - checkLimit(); - this.inferred = false; - this.hasUnbound = hasUnbound || this instanceof Unbound; + this(Arrays.asList(children)); } protected Expression(List children) { @@ -157,6 +112,7 @@ protected Expression(List children, boolean inferred) { this.width = child.width; this.compareWidthAndDepth = child.compareWidthAndDepth && supportCompareWidthAndDepth(); this.fastChildrenHashCode = child.fastChildrenHashCode() + 1; + hasUnbound = child.hasUnbound(); break; case 2: Expression left = children.get(0); @@ -166,6 +122,7 @@ protected Expression(List children, boolean inferred) { this.compareWidthAndDepth = left.compareWidthAndDepth && right.compareWidthAndDepth && supportCompareWidthAndDepth(); this.fastChildrenHashCode = left.fastChildrenHashCode() + right.fastChildrenHashCode() + 2; + hasUnbound = left.hasUnbound() || right.hasUnbound(); break; default: int maxChildDepth = 0; diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/ExpressionUnboundTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/ExpressionUnboundTest.java new file mode 100644 index 00000000000000..78fc821f336fe4 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/ExpressionUnboundTest.java @@ -0,0 +1,233 @@ +// 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.expressions; + +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.IntegerType; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.List; + +/** + * Test for Expression hasUnbound assignment logic. + */ +public class ExpressionUnboundTest { + + /** + * Mock bound expression for testing. + */ + private static class MockBoundExpression extends Expression { + public MockBoundExpression() { + super(); + } + + public MockBoundExpression(Expression... children) { + super(children); + } + + public MockBoundExpression(List children) { + super(children); + } + + @Override + public R accept(ExpressionVisitor visitor, C context) { + return null; + } + + @Override + public DataType getDataType() { + return IntegerType.INSTANCE; + } + + @Override + public boolean nullable() { + return false; + } + + @Override + public Expression withChildren(List children) { + return new MockBoundExpression(children); + } + + @Override + protected String computeToSql() { + return toSql(); + } + } + + /** + * Mock unbound expression for testing. + */ + private static class MockUnboundExpression extends Expression { + public MockUnboundExpression() { + super(); + } + + public MockUnboundExpression(Expression... children) { + super(children); + } + + public MockUnboundExpression(List children) { + super(children); + } + + @Override + public R accept(ExpressionVisitor visitor, C context) { + return null; + } + + @Override + public DataType getDataType() { + return IntegerType.INSTANCE; + } + + @Override + public boolean nullable() { + return false; + } + + @Override + public Expression withChildren(List children) { + return new MockUnboundExpression(children); + } + + @Override + protected String computeToSql() { + return toSql(); + } + + // Override hasUnbound to return true for testing + @Override + public boolean hasUnbound() { + return true; + } + } + + /** + * Test case 1: Single bound child (line 115 logic). + * When there's one bound child, parent should be bound. + */ + @Test + public void testSingleBoundChild() { + // Create a bound child expression + MockBoundExpression boundChild = new MockBoundExpression(); + Assertions.assertFalse(boundChild.hasUnbound(), "Bound child should not have unbound"); + + // Create parent with single bound child + MockBoundExpression parent = new MockBoundExpression(boundChild); + + // Verify line 115: hasUnbound = child.hasUnbound(); + Assertions.assertFalse(parent.hasUnbound(), "Parent with bound child should not have unbound"); + } + + /** + * Test case 2: Single unbound child (line 115 logic). + * When there's one unbound child, parent should be unbound. + */ + @Test + public void testSingleUnboundChild() { + // Create an unbound child expression + MockUnboundExpression unboundChild = new MockUnboundExpression(); + Assertions.assertTrue(unboundChild.hasUnbound(), "Unbound child should have unbound"); + + // Create parent with single unbound child + MockBoundExpression parent = new MockBoundExpression(unboundChild); + + // Verify line 115: hasUnbound = child.hasUnbound(); + Assertions.assertTrue(parent.hasUnbound(), "Parent with unbound child should have unbound"); + } + + /** + * Test case 3: Two bound children (line 125 logic). + * When both children are bound, parent should be bound. + */ + @Test + public void testTwoBoundChildren() { + // Create two bound child expressions + MockBoundExpression leftChild = new MockBoundExpression(); + MockBoundExpression rightChild = new MockBoundExpression(); + Assertions.assertFalse(leftChild.hasUnbound(), "Left bound child should not have unbound"); + Assertions.assertFalse(rightChild.hasUnbound(), "Right bound child should not have unbound"); + + // Create parent with two bound children + MockBoundExpression parent = new MockBoundExpression(leftChild, rightChild); + + // Verify line 125: hasUnbound = left.hasUnbound() || right.hasUnbound(); + Assertions.assertFalse(parent.hasUnbound(), "Parent with two bound children should not have unbound"); + } + + /** + * Test case 4: Left unbound, right bound (line 125 logic). + * When left child is unbound, parent should be unbound. + */ + @Test + public void testLeftUnboundRightBound() { + // Create unbound left and bound right child + MockUnboundExpression leftChild = new MockUnboundExpression(); + MockBoundExpression rightChild = new MockBoundExpression(); + Assertions.assertTrue(leftChild.hasUnbound(), "Left unbound child should have unbound"); + Assertions.assertFalse(rightChild.hasUnbound(), "Right bound child should not have unbound"); + + // Create parent with left unbound, right bound + MockBoundExpression parent = new MockBoundExpression(leftChild, rightChild); + + // Verify line 125: hasUnbound = left.hasUnbound() || right.hasUnbound(); + Assertions.assertTrue(parent.hasUnbound(), "Parent with left unbound child should have unbound"); + } + + /** + * Test case 5: Left bound, right unbound (line 125 logic). + * When right child is unbound, parent should be unbound. + */ + @Test + public void testLeftBoundRightUnbound() { + // Create bound left and unbound right child + MockBoundExpression leftChild = new MockBoundExpression(); + MockUnboundExpression rightChild = new MockUnboundExpression(); + Assertions.assertFalse(leftChild.hasUnbound(), "Left bound child should not have unbound"); + Assertions.assertTrue(rightChild.hasUnbound(), "Right unbound child should have unbound"); + + // Create parent with left bound, right unbound + MockBoundExpression parent = new MockBoundExpression(leftChild, rightChild); + + // Verify line 125: hasUnbound = left.hasUnbound() || right.hasUnbound(); + Assertions.assertTrue(parent.hasUnbound(), "Parent with right unbound child should have unbound"); + } + + /** + * Test case 6: Both children unbound (line 125 logic). + * When both children are unbound, parent should be unbound. + */ + @Test + public void testTwoUnboundChildren() { + // Create two unbound child expressions + MockUnboundExpression leftChild = new MockUnboundExpression(); + MockUnboundExpression rightChild = new MockUnboundExpression(); + Assertions.assertTrue(leftChild.hasUnbound(), "Left unbound child should have unbound"); + Assertions.assertTrue(rightChild.hasUnbound(), "Right unbound child should have unbound"); + + // Create parent with two unbound children + MockBoundExpression parent = new MockBoundExpression(leftChild, rightChild); + + // Verify line 125: hasUnbound = left.hasUnbound() || right.hasUnbound(); + Assertions.assertTrue(parent.hasUnbound(), "Parent with two unbound children should have unbound"); + } +}