[
https://issues.apache.org/jira/browse/CALCITE-6322?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17839069#comment-17839069
]
Mihai Budiu commented on CALCITE-6322:
--------------------------------------
Indeed, the two PRs address the same problem.
I wasn't aware of the previous one.
I think the solution in [CALCITE-5860] is incomplete when overflows occur.
The implementation in this PR essentially implements the same functionality,
but divided into 3 functions, handling separately casts from integers, floating
point, and decimal (all to a decimal type). After converting the value using
the correct rounding rules, this PR also checks that the resulting value is
representable using the target type. If not a (runtime) error is produced.
Regarding the rounding mode, elsewhere in Calcite, e.g., when decimals are
converted to integers, a function like BigDecimal.longValue() is used, which
implicitly rounds down. I personally find this strange, but is legal according
to the SQL standard as far as I can tell. There is no spec in Calcite saying
how rounding should be done in type conversions, and that is a source of
problems, since people who implement the rounding don't have a guideline. This
PR uses ROUNDING_MODE.Floor to be consistent with these other cases. This is
another difference from the earlier PR.
If we want a different rounding mode, I think we should file an issue,
*document* the desired behavior, and write tests that ensure the expected
behavior is implemented. But this may be disruptive to other projects that use
Calcite which have come to rely on the current rounding behavior.
There are several other PRs which handle bugs in arithmetic. I have open 3 PRs
for [CALCITE-3522] (DECIMAL limited to 64 bits), [CALCITE-2067] (Evaluation of
constant expressions with FP results), and [CALCITE-6169] (semantics of casts
in compile-time evaluation). And these won't be enough, there is no PR handling
overflows in INTEGER casts, I plan to work on that too. Each of these is
solving a slightly different set of BUGs. We should double-check which BUGs
that disable tests in SqlOperatorTests are affected, my hope is that we will be
able to close all the ones related to basic arithmetic soon.
Unfortunately my fix uncovers a different bug: CALCITE-6328, which was masked
by the current bug being solved.
My first submission for this PR was in December, so I confess that I no longer
remember exactly which BUGs I have addressed in which PR. But I think we can
make sure that we remove BUGs by having a round of cleanup PRs solely devoted
to removing BUGs related to arithmetic, so we don't have to do it right now
necessarily. I am planning to work on that too, but that work is blocked by
these open PRs.
The earlier issue makes it clear that this problem needs to be solved. What is
the best way to reach an accepted solution? This PR changes many files, but the
only logic changes are in RexToLixTranslator, BuitInMethod, SqlTypeUtil, and
Primitive. totaling less than 90 lines of code. Everything else is test cases
that are affected.
> Casts to DECIMAL types are ignored
> ----------------------------------
>
> Key: CALCITE-6322
> URL: https://issues.apache.org/jira/browse/CALCITE-6322
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.36.0
> Reporter: Mihai Budiu
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.37.0
>
>
> The following SqlOperatorTest fails:
> {code:java}
> f.checkScalar("CAST(1.123 AS DECIMAL(4, 0))", "1.0", "DECIMAL(4, 0) NOT
> NULL");
> {code}
> The result computed by Calcite is 1.123, ignoring the scale of the DECIMAL
> result.
> Spark, Postgres, MySQL all return 1.0.
> I have marked this as a major bug.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
