github-actions[bot] commented on code in PR #64577:
URL: https://github.com/apache/doris/pull/64577#discussion_r3423132180
##########
fe/fe-common/src/main/java/org/apache/doris/common/FractionalFormat.java:
##########
@@ -18,18 +18,20 @@
package org.apache.doris.common;
import java.math.BigDecimal;
-import java.math.RoundingMode;
/**
* Util class for float/double to string.
*/
public class FractionalFormat {
Review Comment:
This changes FE's FLOAT rendering contract even though the BE float path is
unchanged. FE callers pass `precision = 7` for FLOAT, and before this code the
`BigDecimal/setScale` path rounded to `FLT_DIG + 1` significant digits,
matching `CastToString::_fast_to_buffer<float>` (`{:.{}g}` with `digits10 +
1`). `Float.toString` is shortest round-trip instead and can emit more digits;
for example `1.2345678f` becomes `1.2345678` here, while the BE runtime cast
still prints `1.234568`. That means a constant-folded `cast(cast(1.2345678 as
float) as string)` can disagree with reading the same FLOAT value from a table.
Please keep the `precision = 7` path on the previous 7-significant-digit
formatting unless the BE float path and regression coverage are changed
together.
##########
be/src/exprs/function/cast/cast_to_string.h:
##########
@@ -118,8 +118,9 @@ struct CastToString {
end = fmt::format_to(buffer, FMT_COMPILE("{:.{}g}"), value,
std::numeric_limits<float>::digits10 + 1);
} else {
- end = fmt::format_to(buffer, FMT_COMPILE("{:.{}g}"), value,
- std::numeric_limits<double>::digits10 +
1);
+ // shortest round-trip, fixed-precision %g would expose
IEEE-754
+ // residual error e.g. round(23900/293, 2) ->
"81.56999999999999".
+ end = fmt::format_to(buffer, "{}", value);
Review Comment:
This is the per-row double serialization path, and the old code kept the
format string compiled. The new default format can still use the same hot-path
pattern; using plain `"{}"` parses the format for every value. Please keep the
compiled format here.
```suggestion
end = fmt::format_to(buffer, FMT_COMPILE("{}"), value);
```
##########
fe/fe-common/src/main/java/org/apache/doris/common/FractionalFormat.java:
##########
@@ -18,18 +18,20 @@
package org.apache.doris.common;
import java.math.BigDecimal;
-import java.math.RoundingMode;
/**
* Util class for float/double to string.
*/
public class FractionalFormat {
+ // FLT_DIG + 1; precision <= this value means float-equivalent rendering.
Review Comment:
The DOUBLE path is shortest-round-trip only inside this fixed-notation
branch. Values with `exponent >= precision` or `exponent < -4` still fall
through to `String.format(sciFormat, value)` below, which is the old `%.15E`
for double. After the BE change, `_fast_to_buffer<double>` uses shortest
formatting for all magnitudes, so FE folding can still disagree for scientific
values requiring 17 digits; for example `1.2345678901234567e20` is rounded by
the FE `sciFormat` branch to `1.234567890123457e+20` instead of preserving the
shortest value. Please apply the same shortest-round-trip logic to the
scientific branch too, normalizing to Doris' expected `e` spelling, or BE/FE
cast-to-string semantics remain inconsistent.
##########
regression-test/suites/query_p0/sql_functions/math_functions/test_round.groovy:
##########
@@ -325,4 +325,66 @@ suite("test_round") {
qt_sql """
SELECT dfloor(col1, col2) AS r FROM test_dfloor_null order by id;
"""
+
+ sql """ DROP TABLE IF EXISTS test_round_float_print; """
+ sql """
+ CREATE TABLE test_round_float_print (
+ id INT,
+ a_int BIGINT,
+ b_int BIGINT,
+ d_dou DOUBLE,
+ d_flo FLOAT,
+ d_p1 DOUBLE,
+ d_p2 DOUBLE
+ ) DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1");
+ """
+ sql """
+ INSERT INTO test_round_float_print VALUES
+ (1, 23900, 293,
+ cast(23900 as double) / cast(293 as double),
+ cast(cast(23900 as double) / cast(293 as double) as float),
+ cast(0.1 as double),
+ cast(0.2 as double)),
+ (2, 23800, 293, 0, 0, 0, 0),
+ (3, 22400, 277, 0, 0, 0, 0),
+ (4, 45800, 486, 0, 0, 0, 0),
+ (5, 103300, 1101, 0, 0, 0, 0);
+ """
+
+ qt_round_div_int_int """
+ SELECT cast(round(a_int / b_int, 2) as varchar(32))
+ FROM test_round_float_print WHERE id = 1 ORDER BY id;
+ """
+ qt_round_div_double """
+ SELECT cast(round(cast(a_int as double) / cast(b_int as double), 2) as
varchar(32))
+ FROM test_round_float_print WHERE id = 1 ORDER BY id;
+ """
+ qt_round_double_col """
+ SELECT cast(round(d_dou, 2) as varchar(32))
+ FROM test_round_float_print WHERE id = 1 ORDER BY id;
+ """
+ qt_float_print_imprecise_sum """
+ SELECT cast(d_p1 + d_p2 as varchar(32))
+ FROM test_round_float_print WHERE id = 1 ORDER BY id;
+ """
+ qt_round_div_int_int_extra """
+ SELECT id, cast(round(a_int / b_int, 2) as varchar(32))
+ FROM test_round_float_print
+ WHERE id BETWEEN 2 AND 5
+ ORDER BY id;
+ """
+ qt_round_float_col """
+ SELECT cast(round(d_flo, 2) as varchar(32))
+ FROM test_round_float_print WHERE id = 1 ORDER BY id;
+ """
+
+ sql """ DROP TABLE IF EXISTS test_round_float_print; """
Review Comment:
The added regression case drops `test_round_float_print` after running it.
The repo's regression-test rules in `AGENTS.md` require dropping tables before
use, not after, so failed cases leave their data for debugging. This test
already has the pre-test `DROP TABLE IF EXISTS`; please remove the trailing
drop.
--
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]
