yujun777 commented on code in PR #64335:
URL: https://github.com/apache/doris/pull/64335#discussion_r3503178829
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -422,40 +421,6 @@ public static <S extends NamedExpression> S
selectMinimumColumn(Collection<S> sl
}
Review Comment:
Fixed in acb3634 — the duplicate `/**` has been removed.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/SimplifyAggGroupByTest.java:
##########
@@ -156,4 +169,231 @@ void testisBinaryArithmeticSlot() {
Divide divide = new Divide(id, Literal.of(2));
Assertions.assertTrue(SimplifyAggGroupBy.isBinaryArithmeticSlot(divide));
}
+
+ // ========== new tests for injectivity checks ==========
+
+ @Test
+ void testMultiplyByZero() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(id, Literal.of(0))));
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(Literal.of(0), id)));
+ }
+
+ @Test
+ void testDivideZeroNumerator() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Divide(Literal.of(0), id)));
+ }
+
+ @Test
+ void testDivideByZero() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Divide(id, Literal.of(0))));
+ }
+
+ @Test
+ void testNullLiteral() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Add(id, NullLiteral.INSTANCE)));
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(id, NullLiteral.INSTANCE)));
+ }
+
+ @Test
+ void testMultiplyWithDoubleLiteral() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(id, new DoubleLiteral(0.1))));
+ }
+
+ @Test
+ void testDivideWithDoubleLiteral() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Divide(id, new DoubleLiteral(2.0))));
+ }
+
+ @Test
+ void testMultiplyWithFloatSlot() {
+ Slot floatSlot = new SlotReference("f", FloatType.INSTANCE);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(floatSlot, Literal.of(2))));
+ }
+
+ @Test
+ void testMultiplyDoubleSlotWithIntLiteral() {
+ Slot doubleSlot = new SlotReference("d", DoubleType.INSTANCE);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(doubleSlot, Literal.of(2))));
+ }
+
+ @Test
+ void testAddWithDoubleLiteral() {
+ // Float/double arithmetic may be imprecise, reject for all ops
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Add(id, new DoubleLiteral(1.0))));
+ }
+
+ @Test
+ void testAddWithFloatLiteral() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Add(id, new FloatLiteral(1.0f))));
+ }
+
+ @Test
+ void testSubtractWithDoubleLiteral() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertFalse(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Subtract(id, new DoubleLiteral(1.0))));
+ }
+
+ @Test
+ void testMultiplyWithDecimalLiteral() {
+ // Small decimal multiply should pass (precision fits)
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertTrue(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Multiply(id, new DecimalLiteral(new BigDecimal("2.0")))));
+ }
+
+ @Test
+ void testDivideWithDecimalLiteral() {
+ // Divide with decimal: precision overflow too extreme to worry about
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertTrue(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Divide(id, new DecimalLiteral(new BigDecimal("2.0")))));
+ }
+
+ @Test
+ void testAddWithDecimalLiteral() {
+ // Add/Subtract with decimal are exact, should pass
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertTrue(SimplifyAggGroupBy.isBinaryArithmeticSlot(
+ new Add(id, new DecimalLiteral(new BigDecimal("1.0")))));
+ }
+
+ // ========== tests for isInjectiveCastTo ==========
+
+ @Test
+ void testIntegerWidening() {
+
Assertions.assertTrue(TinyIntType.INSTANCE.isInjectiveCastTo(IntegerType.INSTANCE));
+
Assertions.assertTrue(IntegerType.INSTANCE.isInjectiveCastTo(BigIntType.INSTANCE));
+
Assertions.assertFalse(IntegerType.INSTANCE.isInjectiveCastTo(TinyIntType.INSTANCE));
+
Assertions.assertFalse(BigIntType.INSTANCE.isInjectiveCastTo(IntegerType.INSTANCE));
+ }
+
+ @Test
+ void testDecimalWidening() {
+ Assertions.assertTrue(DecimalV3Type.createDecimalV3Type(5, 2)
+ .isInjectiveCastTo(DecimalV3Type.createDecimalV3Type(10, 4)));
+ Assertions.assertFalse(DecimalV3Type.createDecimalV3Type(10, 4)
+ .isInjectiveCastTo(DecimalV3Type.createDecimalV3Type(5, 2)));
+ }
+
+ @Test
+ void testIntegralToDecimalWidening() {
+ Assertions.assertTrue(TinyIntType.INSTANCE
+ .isInjectiveCastTo(DecimalV3Type.createDecimalV3Type(10, 0)));
+ // BigInt has 19 digits, DECIMAL(5,0) only has 5 integer digits
+ Assertions.assertFalse(BigIntType.INSTANCE
+ .isInjectiveCastTo(DecimalV3Type.createDecimalV3Type(5, 0)));
+ }
+
+ @Test
+ void testCrossFamilyRejected() {
+
Assertions.assertFalse(IntegerType.INSTANCE.isInjectiveCastTo(FloatType.INSTANCE));
+
Assertions.assertFalse(FloatType.INSTANCE.isInjectiveCastTo(IntegerType.INSTANCE));
+
Assertions.assertFalse(IntegerType.INSTANCE.isInjectiveCastTo(DoubleType.INSTANCE));
+ }
+
+ // ========== tests for canExtractSlot ==========
+
+ @Test
+ void testCanExtractSlotBare() {
+ Slot id = scan1.getOutput().get(0);
+ Assertions.assertTrue(SimplifyAggGroupBy.canExtractSlot(id));
+ }
+
Review Comment:
The `canExtractSlot()` unit tests cover the cast-path logic adequately.
Adding full PlanChecker integration tests for the cast scenario would be
redundant since the rule logic is the same regardless of whether the cast was
added by the analyzer or present in the original SQL. The existing PlanChecker
integration tests for multiply-by-zero, null-literal, and double-literal
already validate the full rewrite path.
--
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]
