abhinav-phi commented on code in PR #68733:
URL: https://github.com/apache/doris/pull/68733#discussion_r4296947595
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/FractionalLiteral.java:
##########
@@ -53,7 +53,11 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
if (!isFinite) {
throw new CastException(String.format("%s can't cast to %s in
strict mode.", getValue(), targetType));
}
- BigDecimal decimal = new BigDecimal(value.toString());
+ // BE converts the binary value, not the shortest decimal
representation of
+ // the float/double, so a magnitude such as 1e38 must keep the
double's exact value
+ BigDecimal decimal = value instanceof Double || value instanceof
Float
Review Comment:
fixed in bd9454c5. the overflow precheck now runs on the binary value rather
than on the shortest decimal printing of it:
BigDecimal decimal = value instanceof Double || value instanceof Float
? new BigDecimal(((Number) value).doubleValue())
: new BigDecimal(value.toString());
if (numericOverflow(decimal, targetType)) {
throw new CastException(...);
}
that same BigDecimal is what the `setScale(0, DOWN)` then produces the
integral literal from, so the value being range checked and the value being
written out are one value instead of two. `Literal.numericOverflow` got the
matching overload so the string based entry point can not reintroduce the gap.
the tests added for this are the exactly representable edges,
`-9223372036854775808.0` to BIGINT and `-2147483648.0f` to INT. both print
outside the target range while being exactly representable in it, which is the
case a printed value check gets wrong in the accepting direction.
to be straight about coverage, i did not add a 1e20f to LARGEINT case here
because that specific path goes through the FLOAT to DOUBLE widening, which is
the second note below and the subject of #68730.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/FractionalLiteral.java:
##########
@@ -53,7 +53,11 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
if (!isFinite) {
throw new CastException(String.format("%s can't cast to %s in
strict mode.", getValue(), targetType));
}
- BigDecimal decimal = new BigDecimal(value.toString());
+ // BE converts the binary value, not the shortest decimal
representation of
+ // the float/double, so a magnitude such as 1e38 must keep the
double's exact value
+ BigDecimal decimal = value instanceof Double || value instanceof
Float
+ ? new BigDecimal(((Number) value).doubleValue())
Review Comment:
this one is genuinely not addressed by this pr and i would rather say so
than let it look covered. the line you are pointing at is the FLOAT to DOUBLE
widening in `FloatLiteral.uncheckedCastTo`, and that exact line is what sibling
pr #68730 changes, to `new DoubleLiteral((double) value)` with a comment about
matching the BE Float32 to Float64 cast.
i left it alone here so two of my branches are not rewriting the same line
against each other. if #68730 lands first this is resolved by it. if you would
rather this branch carry it too, or carry it instead of that one, tell me which
shape you want and i will rework this branch to match.
what this pr does fix is the printed versus binary value on the cast to
integral path, covered in the note above on line 58.
--
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]
