xy720 commented on code in PR #64622: URL: https://github.com/apache/doris/pull/64622#discussion_r3520966570
########## 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: Added qt_round_overflow_1e16 and qt_round_overflow_1e300 to the regression suite. ########## 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: Good call, added a comment above the constant. ########## 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: Added a roundDoubleWithFoldedIntExpressionReturnsDecimal test. ########## 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: Reworked the description in both CN and EN. ########## 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: Thanks for the thorough read! I think there may be a small misread here — the outer condition is still isFloatLikeType(), unchanged. Only the inner nested check on added specifically to gate the DECIMAL(30, 15) reroute for DOUBLE-only. So the routing is unchanged for FLOAT: - round(FLOAT) — returns `(DOUBLE) → DOUBLE ` - round(FLOAT, INT) — returns `(DOUBLE, INT) → DOUBLE` I think it is necessary to narrow the scope to double-only, because casting float to DECIMAL(14, 7) can lead to numerical overflow beyond the range of 1e7. This overflow range is smaller then 1e15(for double), making it easier for users to trigger. I'll leave the outer condition and the class javadoc as-is unless you spot something else. ########## 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: Added if (folded == null) return false; -- 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]
