zabetak commented on code in PR #3326:
URL: https://github.com/apache/calcite/pull/3326#discussion_r1279110880
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -774,10 +779,15 @@ public static Expression translateLiteral(
return Expressions.constant(bd, javaClass);
}
assert javaClass == BigDecimal.class;
- return Expressions.new_(BigDecimal.class,
- Expressions.constant(
+ Expression expression =
+ Expressions.new_(
+ BigDecimal.class, constant(
requireNonNull(bd,
() -> "value for " + literal).toString()));
+ if (type.getScale() != bd.scale()) {
+ expression = adjustDecimalScale(type.getScale(), expression);
+ }
+ return expression;
Review Comment:
If we have a literal why do we need to adjust the scale? Does the
`BigDecimal(String)` constructor pick a wrong scale for certain inputs? If
that's the case can you please provide some examples with such inputs?
Which of the existing unit tests are affected by this change?
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -625,7 +627,10 @@ private static Expression checkExpressionPadTruncate(
default:
return operand;
}
-
+ case DECIMAL:
+ return sourceType.getScale() == targetType.getScale()
+ ? operand
+ : adjustDecimalScale(targetType.getScale(), operand);
Review Comment:
Most of the numeric conversions are happening inside `EnumUtils.convert`
method so I am wondering if `checkExpressionPadTruncate` is the right place to
apply the changes. Can you please elaborate on why the changes should be done
here and not for instance inside `EnumUtils`?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6421,7 +6421,11 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkType("atanh('abc')", "DOUBLE NOT NULL");
f.checkScalarApprox("atanh(0.76159416)", "DOUBLE NOT NULL",
isWithin(1d, 0.0001d));
+ f.checkScalarApprox("atanh(cast(-0.1 as decimal(19, 0)))", "DOUBLE NOT
NULL",
Review Comment:
Do we need `atanh` and `ceil` to hit the problem or it is sufficient to use
`CAST`? If the problem is reproable simply with `CAST` then we should simply
add test cases for that.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -3315,6 +3315,13 @@ private void
checkReduceNullableToNotNull(ReduceExpressionsRule<?> rule) {
checkReduceNullableToNotNull(rule);
}
+ @Test void testReduceCastDecimal() {
+ final String sql = "SELECT CAST( ( (2) / SQRT(3.0) ) AS NUMERIC(18, 0) ) *
SQRT(3.0)";
+ sql(sql).withRule(CoreRules.PROJECT_REDUCE_EXPRESSIONS)
+ .withRelBuilderSimplify(true)
+ .check();
+ }
+
Review Comment:
Usually we add tests in this class to demonstrate that certain rules will
apply and have the desired result. I don't see any changes in
`PROJECT_REDUCE_EXPRESSIONS` rule so this test seems slightly
misplaced/redundant.
The problem seems to be around the CAST function so a more appropriate place
would be the `operator.iq` file and/or `SqlOperatorTest`.
Also I am wondering if the expression `CAST( ( (2) / SQRT(3.0) ) AS
NUMERIC(18, 0) ) * SQRT(3.0)` is minimal. Is there a smaller case that
demonstrates the problem? If yes then we should use that one.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -838,6 +848,13 @@ public static Expression translateLiteral(
return Expressions.constant(value2, javaClass);
}
+ private static MethodCallExpression adjustDecimalScale(int scale,
+ Expression expression) {
+ assert expression.getType() == BigDecimal.class;
+ return Expressions.call(expression,
BuiltInMethod.BIG_DECIMAL_SET_SCALE.method,
+ Expressions.constant(scale),
Expressions.constant(RoundingMode.HALF_UP));
Review Comment:
Why did we pick the `RoundingMode.HALF_UP`? Is it something mandated by the
SQL standard? Is it something commonly used by other DBMS? Do we need to
document this behavior in the website?
--
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]
