morrySnow commented on code in PR #64622:
URL: https://github.com/apache/doris/pull/64622#discussion_r3491139620
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRound.java:
##########
@@ -18,26 +18,86 @@
package org.apache.doris.nereids.trees.expressions.functions;
import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
+import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.coercion.Int32OrLessType;
+import org.apache.doris.qe.ConnectContext;
+import java.math.BigInteger;
+import java.util.ArrayList;
import java.util.List;
/**
- * if argument 0 is float or double, we should return double signature for
round like function.
+ * Signature search for round-like functions (round, round_bankers, ceil,
floor, truncate).
*/
public interface SearchSignatureForRound extends ExplicitlyCastableSignature {
+
+ int DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15;
+
@Override
default FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
List<Expression> arguments = getArguments();
if (arguments.get(0).getDataType().isFloatLikeType()) {
if (arguments.size() == 1) {
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE);
Review Comment:
The outer condition changed from `isFloatLikeType()` to `isDoubleType()`.
Previously, both FLOAT and DOUBLE inputs would be forced to a `(DOUBLE) →
DOUBLE` signature (single-arg) or `(DOUBLE, INT) → DOUBLE` (two-arg). Now, only
DOUBLE inputs get this treatment — FLOAT inputs fall through to the standard
signature search.
For two-arg FLOAT this is probably fine (standard search resolves to
`(DOUBLE, INT) → DOUBLE` via implicit promotion). But for single-arg FLOAT, the
standard search might find `(FLOAT) → BIGINT` instead of `(DOUBLE) → DOUBLE`,
which changes the return type. The old class javadoc says _"if argument 0 is
float or double"_ — if the narrowing to DOUBLE-only is intentional, please
update the javadoc to match.
For reference, the DECIMAL reroute itself is scoped to DOUBLE-only via the
inner `isDoubleType()` check, which is correct since the DECIMAL(30,15) cast
target is sized for DOUBLE's precision.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRound.java:
##########
@@ -18,26 +18,86 @@
package org.apache.doris.nereids.trees.expressions.functions;
import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
+import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.coercion.Int32OrLessType;
+import org.apache.doris.qe.ConnectContext;
+import java.math.BigInteger;
+import java.util.ArrayList;
import java.util.List;
/**
- * if argument 0 is float or double, we should return double signature for
round like function.
+ * Signature search for round-like functions (round, round_bankers, ceil,
floor, truncate).
*/
public interface SearchSignatureForRound extends ExplicitlyCastableSignature {
+
+ int DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15;
+
@Override
default FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
List<Expression> arguments = getArguments();
if (arguments.get(0).getDataType().isFloatLikeType()) {
if (arguments.size() == 1) {
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE);
} else if (arguments.size() == 2) {
+ if (arguments.get(0).getDataType().isDoubleType()
Review Comment:
The inner `arguments.get(0).getDataType().isDoubleType()` check is redundant
— we are already inside the outer `if
(arguments.get(0).getDataType().isDoubleType())` block. Consider removing the
duplicate check for readability.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRound.java:
##########
@@ -18,26 +18,86 @@
package org.apache.doris.nereids.trees.expressions.functions;
import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
+import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.coercion.Int32OrLessType;
+import org.apache.doris.qe.ConnectContext;
+import java.math.BigInteger;
+import java.util.ArrayList;
import java.util.List;
/**
- * if argument 0 is float or double, we should return double signature for
round like function.
+ * Signature search for round-like functions (round, round_bankers, ceil,
floor, truncate).
*/
public interface SearchSignatureForRound extends ExplicitlyCastableSignature {
+
+ int DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15;
+
@Override
default FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
List<Expression> arguments = getArguments();
if (arguments.get(0).getDataType().isFloatLikeType()) {
if (arguments.size() == 1) {
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE);
} else if (arguments.size() == 2) {
+ if (arguments.get(0).getDataType().isDoubleType()
+ && isOptedIntoDecimalReroute()
+ && isNonNegativeIntegerLiteralAtMost(arguments.get(1),
+ DOUBLE_DECIMAL_RESULT_MAX_SCALE)) {
+ return ExplicitlyCastableSignature.super.searchSignature(
+ withoutFloatLikeReturnTypes(signatures));
+ }
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE,
IntegerType.INSTANCE);
}
}
return ExplicitlyCastableSignature.super.searchSignature(signatures);
}
+
+ static boolean isOptedIntoDecimalReroute() {
+ ConnectContext ctx = ConnectContext.get();
+ return ctx != null &&
ctx.getSessionVariable().roundDoubleReturnsDecimalForConstScale;
+ }
+
+ /**
+ * True iff scale folds to an integer literal whose value lies in the
closed range
+ * [0, maxValue].
+ */
+ static boolean isNonNegativeIntegerLiteralAtMost(Expression scale, int
maxValue) {
+ Expression folded = scale;
+ if (!folded.isLiteral() && !folded.isSlot()) {
+ ExpressionRewriteContext ctx = new
ExpressionRewriteContext(CascadesContext.initTempContext());
+ folded = FoldConstantRuleOnFE.evaluate(folded, ctx);
+ }
+ Expression unwrapped = folded;
+ if (unwrapped instanceof Cast && unwrapped.child(0).isLiteral()
+ && unwrapped.child(0).getDataType() instanceof
Int32OrLessType) {
+ unwrapped = unwrapped.child(0);
+ }
+ if (!(unwrapped instanceof IntegerLikeLiteral)) {
+ return false;
+ }
+ Number number = ((IntegerLikeLiteral) unwrapped).getNumber();
+ BigInteger value = (number instanceof BigInteger)
+ ? (BigInteger) number
+ : BigInteger.valueOf(number.longValue());
+ return value.signum() >= 0 &&
value.compareTo(BigInteger.valueOf(maxValue)) <= 0;
+ }
+
+ /** Drop signatures whose return type is a float-like type, so the search
falls onto decimal. */
+ static List<FunctionSignature>
withoutFloatLikeReturnTypes(List<FunctionSignature> signatures) {
+ List<FunctionSignature> result = new ArrayList<>(signatures.size());
+ for (FunctionSignature signature : signatures) {
+ if (!signature.returnType.isFloatLikeType()) {
Review Comment:
This follows the same fold-then-unwrap-Cast pattern used in
`ComputePrecisionForRound`. Consider also adding a `null`-safety guard on the
folded result — while `FoldConstantRuleOnFE.evaluate` should return the input
unchanged (not null) when folding fails, an explicit guard would make the code
more robust and self-documenting:
```java
if (folded == null) {
return false;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRound.java:
##########
@@ -18,26 +18,86 @@
package org.apache.doris.nereids.trees.expressions.functions;
import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
+import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.coercion.Int32OrLessType;
+import org.apache.doris.qe.ConnectContext;
+import java.math.BigInteger;
+import java.util.ArrayList;
import java.util.List;
/**
- * if argument 0 is float or double, we should return double signature for
round like function.
+ * Signature search for round-like functions (round, round_bankers, ceil,
floor, truncate).
*/
public interface SearchSignatureForRound extends ExplicitlyCastableSignature {
+
+ int DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15;
+
@Override
default FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
List<Expression> arguments = getArguments();
if (arguments.get(0).getDataType().isFloatLikeType()) {
if (arguments.size() == 1) {
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE);
} else if (arguments.size() == 2) {
+ if (arguments.get(0).getDataType().isDoubleType()
+ && isOptedIntoDecimalReroute()
+ && isNonNegativeIntegerLiteralAtMost(arguments.get(1),
+ DOUBLE_DECIMAL_RESULT_MAX_SCALE)) {
+ return ExplicitlyCastableSignature.super.searchSignature(
+ withoutFloatLikeReturnTypes(signatures));
+ }
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE,
IntegerType.INSTANCE);
}
}
return ExplicitlyCastableSignature.super.searchSignature(signatures);
}
+
+ static boolean isOptedIntoDecimalReroute() {
Review Comment:
Nit: `DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15` corresponds to the ~15-17
significant decimal digits that IEEE-754 double precision can faithfully
represent. A brief comment explaining why 15 (and not 17 or 18) would help
future maintainers understand the threshold.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1915,6 +1918,23 @@ public void setMaxJoinNumberOfReorder(int
maxJoinNumberOfReorder) {
)
public int decimalOverflowScale = 6;
+ @VarAttrDef.VarAttr(name = ROUND_DOUBLE_RETURNS_DECIMAL_FOR_CONST_SCALE,
Review Comment:
The descriptions (both CN and EN) clearly explain the trade-off. A small
suggestion: explicitly mention the `DECIMAL(30, 15)` target type since that is
the intermediate cast that gates overflow. Users may wonder which DECIMAL
precision/scale is used.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRound.java:
##########
@@ -18,26 +18,86 @@
package org.apache.doris.nereids.trees.expressions.functions;
import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
+import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.coercion.Int32OrLessType;
+import org.apache.doris.qe.ConnectContext;
+import java.math.BigInteger;
+import java.util.ArrayList;
import java.util.List;
/**
- * if argument 0 is float or double, we should return double signature for
round like function.
+ * Signature search for round-like functions (round, round_bankers, ceil,
floor, truncate).
*/
public interface SearchSignatureForRound extends ExplicitlyCastableSignature {
+
+ int DOUBLE_DECIMAL_RESULT_MAX_SCALE = 15;
+
@Override
default FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
List<Expression> arguments = getArguments();
if (arguments.get(0).getDataType().isFloatLikeType()) {
if (arguments.size() == 1) {
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE);
} else if (arguments.size() == 2) {
+ if (arguments.get(0).getDataType().isDoubleType()
+ && isOptedIntoDecimalReroute()
+ && isNonNegativeIntegerLiteralAtMost(arguments.get(1),
+ DOUBLE_DECIMAL_RESULT_MAX_SCALE)) {
+ return ExplicitlyCastableSignature.super.searchSignature(
+ withoutFloatLikeReturnTypes(signatures));
+ }
return
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE,
IntegerType.INSTANCE);
}
}
return ExplicitlyCastableSignature.super.searchSignature(signatures);
}
+
+ static boolean isOptedIntoDecimalReroute() {
+ ConnectContext ctx = ConnectContext.get();
+ return ctx != null &&
ctx.getSessionVariable().roundDoubleReturnsDecimalForConstScale;
+ }
+
+ /**
+ * True iff scale folds to an integer literal whose value lies in the
closed range
+ * [0, maxValue].
+ */
+ static boolean isNonNegativeIntegerLiteralAtMost(Expression scale, int
maxValue) {
+ Expression folded = scale;
+ if (!folded.isLiteral() && !folded.isSlot()) {
+ ExpressionRewriteContext ctx = new
ExpressionRewriteContext(CascadesContext.initTempContext());
+ folded = FoldConstantRuleOnFE.evaluate(folded, ctx);
+ }
+ Expression unwrapped = folded;
+ if (unwrapped instanceof Cast && unwrapped.child(0).isLiteral()
+ && unwrapped.child(0).getDataType() instanceof
Int32OrLessType) {
+ unwrapped = unwrapped.child(0);
+ }
+ if (!(unwrapped instanceof IntegerLikeLiteral)) {
+ return false;
+ }
+ Number number = ((IntegerLikeLiteral) unwrapped).getNumber();
+ BigInteger value = (number instanceof BigInteger)
+ ? (BigInteger) number
+ : BigInteger.valueOf(number.longValue());
+ return value.signum() >= 0 &&
value.compareTo(BigInteger.valueOf(maxValue)) <= 0;
+ }
+
+ /** Drop signatures whose return type is a float-like type, so the search
falls onto decimal. */
+ static List<FunctionSignature>
withoutFloatLikeReturnTypes(List<FunctionSignature> signatures) {
+ List<FunctionSignature> result = new ArrayList<>(signatures.size());
+ for (FunctionSignature signature : signatures) {
+ if (!signature.returnType.isFloatLikeType()) {
+ result.add(signature);
+ }
Review Comment:
The Cast-unwrap logic handles `Cast(IntegerLiteral(N), IntType)`, but there
is a subtle ordering: we unwrap *after* the fold step. If the fold step already
reduces `Cast(IntegerLiteral(3), IntType)` to `IntegerLiteral(3)`, the unwrap
path is never taken. Both paths produce the correct result, but the unwrap path
acts as a safety net when folding doesn't eliminate the Cast. This is fine,
just noting the dual-path design.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRoundTest.java:
##########
@@ -0,0 +1,223 @@
+// 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.functions;
+
+import org.apache.doris.nereids.trees.expressions.Cast;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Ceil;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Floor;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Round;
+import
org.apache.doris.nereids.trees.expressions.functions.scalar.RoundBankers;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Truncate;
+import org.apache.doris.nereids.trees.expressions.literal.BigIntLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.LargeIntLiteral;
+import org.apache.doris.nereids.types.DecimalV3Type;
+import org.apache.doris.nereids.types.DoubleType;
+import org.apache.doris.nereids.types.FloatType;
+import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.qe.ConnectContext;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.math.BigInteger;
+
+public class SearchSignatureForRoundTest {
+
+ private static final DoubleLiteral DOUBLE_VAL = new
DoubleLiteral(81.56996587030717);
+
+ /** Run {@code body} with a fresh ConnectContext whose new opt-in var is
set to {@code optIn}. */
+ private static void withOptIn(boolean optIn, Runnable body) {
+ try (MockedStatic<ConnectContext> mockedContext =
Mockito.mockStatic(ConnectContext.class)) {
+ ConnectContext ctx = new ConnectContext();
+ ctx.getSessionVariable().roundDoubleReturnsDecimalForConstScale =
optIn;
+ mockedContext.when(ConnectContext::get).thenReturn(ctx);
+ body.run();
+ }
+ }
+
+ private static void assertDecimalReturn(int expectedScale, Expression
expr) {
+ Assertions.assertTrue(expr.getDataType() instanceof DecimalV3Type,
+ () -> "expected DecimalV3Type, got " + expr.getDataType());
+ DecimalV3Type t = (DecimalV3Type) expr.getDataType();
+ Assertions.assertEquals(expectedScale, t.getScale(),
+ () -> "expected scale=" + expectedScale + ", got " + t);
+ }
+
+ private static void assertDoubleReturn(Expression expr) {
+ Assertions.assertTrue(expr.getDataType() instanceof DoubleType,
+ () -> "expected DoubleType, got " + expr.getDataType());
+ }
+
+ // ---- opt-in ON: (DOUBLE, non-negative int literal <= 15) routes to
DECIMAL ----
+
+ @Test
+ void roundDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Round(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void roundBankersDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new RoundBankers(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void ceilDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Ceil(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void floorDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Floor(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void truncateDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Truncate(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void zeroScaleAlsoReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(0, new Round(DOUBLE_VAL, new
IntegerLiteral(0))));
+ }
+
+ @Test
+ void roundDoubleAtMaxPreservableScaleReturnsDecimal() {
+ // scale 15 == DOUBLE_DECIMAL.scale.
+ withOptIn(true, () ->
+ assertDecimalReturn(15, new Round(DOUBLE_VAL, new
IntegerLiteral(15))));
+ }
+
+ @Test
+ void roundDoubleWithCastIntLiteralReturnsDecimal() {
+ withOptIn(true, () -> {
+ Cast wrapped = new Cast(new IntegerLiteral(3),
IntegerType.INSTANCE);
+ assertDecimalReturn(3, new Round(DOUBLE_VAL, wrapped));
+ });
+ }
+
+ // ---- opt-in ON but shape doesn't match: stays DOUBLE ----
+
+ @Test
+ void roundDoubleSingleArgStaysDouble() {
+ withOptIn(true, () -> assertDoubleReturn(new Round(DOUBLE_VAL)));
+ }
+
+ @Test
+ void roundDoubleNegativeScaleStaysDouble() {
+ withOptIn(true, () ->
+ assertDoubleReturn(new Round(DOUBLE_VAL, new
IntegerLiteral(-1))));
+ }
+
+ @Test
+ void roundDoubleScaleFromColumnStaysDouble() {
+ // When the scale comes from a column (non-literal), we cannot pick a
+ // fixed decimal scale at plan time, so we keep the original behavior.
+ withOptIn(true, () -> {
+ SlotReference scaleCol = new SlotReference("n",
IntegerType.INSTANCE);
+ assertDoubleReturn(new Round(DOUBLE_VAL, scaleCol));
+ });
+ }
+
+ @Test
+ void roundFloatWithConstScaleStaysDouble() {
+ // FLOAT input keeps the original DOUBLE return path.
+ withOptIn(true, () -> {
+ SlotReference floatCol = new SlotReference("f",
FloatType.INSTANCE);
+ assertDoubleReturn(new Round(floatCol, new IntegerLiteral(2)));
+ });
+ }
+
+ @Test
+ void roundDoubleScaleAboveMaxPreservableStaysDouble() {
+ // scale 17 exceeds DOUBLE_DECIMAL.scale (15).
+ withOptIn(true, () ->
+ assertDoubleReturn(new Round(DOUBLE_VAL, new
IntegerLiteral(17))));
+ }
+
+ @Test
+ void roundDecimalKeepsExistingDecimalSignature() {
+ // The decimal-input path is independent of the new var.
+ withOptIn(true, () -> {
+ DecimalV3Type t = DecimalV3Type.createDecimalV3Type(10, 5);
+ SlotReference dec = new SlotReference("d", t);
+ assertDecimalReturn(2, new Round(dec, new IntegerLiteral(2)));
+ });
+ }
+
+ @Test
+ void roundDoubleBigIntScaleAboveIntRangeStaysDouble() {
+ withOptIn(true, () ->
+ assertDoubleReturn(new Round(DOUBLE_VAL, new
BigIntLiteral(4294967298L))));
+ }
+
+ @Test
+ void roundDoubleBigIntScaleAtIntMaxPlusOneStaysDouble() {
+ withOptIn(true, () ->
+ assertDoubleReturn(new Round(DOUBLE_VAL, new
BigIntLiteral(2147483648L))));
Review Comment:
Consider adding a test for the fold path where the scale argument is a
non-literal, non-slot expression that folds to a constant integer, e.g.:
```java
// ADD(IntegerLiteral(1), IntegerLiteral(2)) folds to IntegerLiteral(3)
new Round(DOUBLE_VAL, new Add(new IntegerLiteral(1), new IntegerLiteral(2)))
```
This would exercise `FoldConstantRuleOnFE.evaluate` in the
`!folded.isLiteral() && !folded.isSlot()` branch.
##########
regression-test/suites/query_p0/sql_functions/math_functions/test_round_double_tail.groovy:
##########
@@ -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.
+
+suite("test_round_double_tail") {
+ sql "set round_double_returns_decimal_for_const_scale = true"
+
+ qt_round_const """ select round(23900/293, 2); """
+ qt_round_bankers_const """ select round_bankers(23900/293, 2); """
+ qt_ceil_const """ select ceil(23900/293, 2); """
+ qt_floor_const """ select floor(23900/293, 2); """
+ qt_truncate_const """ select truncate(23900/293, 2); """
+
+ sql """ DROP TABLE IF EXISTS test_round_double_tail_t; """
+ sql """
+ CREATE TABLE test_round_double_tail_t (
+ id INT,
+ num BIGINT,
+ den BIGINT,
+ d DOUBLE
+ ) DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1");
+ """
+ sql """
+ INSERT INTO test_round_double_tail_t VALUES
+ (1, 239, 293, cast(239 as double) / cast(293 as double) * 100),
+ (2, 458, 486, cast(458 as double) / cast(486 as double) * 100),
+ (3, 1033, 1101, cast(1033 as double) / cast(1101 as double) * 100),
+ (4, 1040, 1101, cast(1040 as double) / cast(1101 as double) * 100),
+ (5, 140, 172, cast(140 as double) / cast(172 as double) * 100);
+ """
+
+ qt_round_col """ SELECT id, round(d, 2) FROM
test_round_double_tail_t ORDER BY id; """
+ qt_round_bankers_col """ SELECT id, round_bankers(d, 2) FROM
test_round_double_tail_t ORDER BY id; """
+ qt_ceil_col """ SELECT id, ceil(d, 2) FROM
test_round_double_tail_t ORDER BY id; """
+ qt_floor_col """ SELECT id, floor(d, 2) FROM
test_round_double_tail_t ORDER BY id; """
+ qt_truncate_col """ SELECT id, truncate(d, 2) FROM
test_round_double_tail_t ORDER BY id; """
+
+ qt_round_concat_pct """
+ SELECT id, concat(round(cast(num as double) * 100 / cast(den as
double), 2), '%')
+ FROM test_round_double_tail_t
+ ORDER BY id;
+ """
+
+ qt_round_single_arg """ SELECT id, round(d) FROM
test_round_double_tail_t ORDER BY id; """
+ qt_round_negative_scale """ SELECT id, round(d, -1) FROM
test_round_double_tail_t ORDER BY id; """
+
+ // Scale from a column also stays double.
+ sql """ DROP TABLE IF EXISTS test_round_double_tail_scale_col; """
+ sql """
+ CREATE TABLE test_round_double_tail_scale_col (
+ id INT,
+ d DOUBLE,
+ n INT
+ ) DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1");
+ """
+ sql """
+ INSERT INTO test_round_double_tail_scale_col VALUES
Review Comment:
The `round_scale_col` query correctly shows the old DOUBLE behavior with
tail digits when the scale comes from a column — this is expected since
column-based scales cannot be resolved at plan time.
Consider also adding a regression test for the overflow case mentioned in
the PR description (e.g., `round(1e16, 2)` or `round(1e300, 2)`) to document
and lock in the NULL/ARITHMETIC_OVERFLOW behavior when the opt-in is enabled.
Even a simple test like:
```groovy
qt_round_overflow """ select round(1e16, 2); """
```
would be valuable for regression prevention.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForRoundTest.java:
##########
@@ -0,0 +1,223 @@
+// 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.functions;
+
+import org.apache.doris.nereids.trees.expressions.Cast;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Ceil;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Floor;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Round;
+import
org.apache.doris.nereids.trees.expressions.functions.scalar.RoundBankers;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Truncate;
+import org.apache.doris.nereids.trees.expressions.literal.BigIntLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.LargeIntLiteral;
+import org.apache.doris.nereids.types.DecimalV3Type;
+import org.apache.doris.nereids.types.DoubleType;
+import org.apache.doris.nereids.types.FloatType;
+import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.qe.ConnectContext;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.math.BigInteger;
+
+public class SearchSignatureForRoundTest {
+
+ private static final DoubleLiteral DOUBLE_VAL = new
DoubleLiteral(81.56996587030717);
+
+ /** Run {@code body} with a fresh ConnectContext whose new opt-in var is
set to {@code optIn}. */
+ private static void withOptIn(boolean optIn, Runnable body) {
+ try (MockedStatic<ConnectContext> mockedContext =
Mockito.mockStatic(ConnectContext.class)) {
+ ConnectContext ctx = new ConnectContext();
+ ctx.getSessionVariable().roundDoubleReturnsDecimalForConstScale =
optIn;
+ mockedContext.when(ConnectContext::get).thenReturn(ctx);
+ body.run();
+ }
+ }
+
+ private static void assertDecimalReturn(int expectedScale, Expression
expr) {
+ Assertions.assertTrue(expr.getDataType() instanceof DecimalV3Type,
+ () -> "expected DecimalV3Type, got " + expr.getDataType());
+ DecimalV3Type t = (DecimalV3Type) expr.getDataType();
+ Assertions.assertEquals(expectedScale, t.getScale(),
+ () -> "expected scale=" + expectedScale + ", got " + t);
+ }
+
+ private static void assertDoubleReturn(Expression expr) {
+ Assertions.assertTrue(expr.getDataType() instanceof DoubleType,
+ () -> "expected DoubleType, got " + expr.getDataType());
+ }
+
+ // ---- opt-in ON: (DOUBLE, non-negative int literal <= 15) routes to
DECIMAL ----
+
+ @Test
+ void roundDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Round(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void roundBankersDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new RoundBankers(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void ceilDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Ceil(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void floorDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Floor(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void truncateDoubleWithConstScaleReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(2, new Truncate(DOUBLE_VAL, new
IntegerLiteral(2))));
+ }
+
+ @Test
+ void zeroScaleAlsoReturnsDecimal() {
+ withOptIn(true, () ->
+ assertDecimalReturn(0, new Round(DOUBLE_VAL, new
IntegerLiteral(0))));
+ }
+
+ @Test
+ void roundDoubleAtMaxPreservableScaleReturnsDecimal() {
+ // scale 15 == DOUBLE_DECIMAL.scale.
+ withOptIn(true, () ->
+ assertDecimalReturn(15, new Round(DOUBLE_VAL, new
IntegerLiteral(15))));
+ }
+
+ @Test
+ void roundDoubleWithCastIntLiteralReturnsDecimal() {
+ withOptIn(true, () -> {
+ Cast wrapped = new Cast(new IntegerLiteral(3),
IntegerType.INSTANCE);
+ assertDecimalReturn(3, new Round(DOUBLE_VAL, wrapped));
+ });
+ }
+
+ // ---- opt-in ON but shape doesn't match: stays DOUBLE ----
+
+ @Test
Review Comment:
Nice test — `Cast(IntegerLiteral(3), IntegerType.INSTANCE)` specifically
validates the Cast-unwrap path in `isNonNegativeIntegerLiteralAtMost`.
--
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]
