github-actions[bot] commented on code in PR #65609:
URL: https://github.com/apache/doris/pull/65609#discussion_r3664689259
##########
be/src/exprs/function/cast/cast_to_string.h:
##########
@@ -114,13 +113,7 @@ struct CastToString {
end = buffer + neg_inf_str_len;
}
} else {
- if constexpr (std::is_same_v<T, float>) {
- 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);
- }
+ end = fmt::format_to(buffer, FMT_COMPILE("{}"), value);
Review Comment:
**Gate the new spelling across rolling upgrades.** This changes scalar-cast
semantics immediately on each upgraded BE, but old and new builds both
advertise `be_exec_version = 10` and this path does not consult runtime/query
compatibility state. During the supported BE-first rollout, a distributed query
can format the same DOUBLE `8.8` as `8.800000000000001` on an old BE and `8.8`
on a new one; `GROUP BY CAST(d AS STRING)` then treats them as different keys.
Please gate the new spelling from the upgraded FE (as the existing
`new_version_unix_timestamp` pattern does), or introduce the appropriate
execution-version transition.
**Also update the remaining reachable baselines.** Once the new mode is
enabled, `query_p0/outfile/test_outfile.groovy` directly selects the generated
`8.8` row while its unchanged `.out` still expects `8.800000000000001`;
`export_p0/outfile/native/test_outfile_native.groovy` has the same stale
spelling in both result blocks. Please regenerate those baselines so the
conditional outfile suites do not fail.
##########
regression-test/suites/variant_p0/load.groovy:
##########
@@ -107,22 +108,24 @@ suite("regression_test_variant", "p0"){
// 2. type confilct cases
def table_name = "type_conflict_resolution"
- create_table table_name
- sql """insert into ${table_name} values (1, '{"c" : "123"}');"""
- sql """insert into ${table_name} values (2, '{"c" : 123}');"""
- sql """insert into ${table_name} values (3, '{"cc" : [123.2]}');"""
- sql """insert into ${table_name} values (4, '{"cc" : [123.1]}');"""
- sql """insert into ${table_name} values (5, '{"ccc" : 123}');"""
- sql """insert into ${table_name} values (6, '{"ccc" : 123321}');"""
- sql """insert into ${table_name} values (7, '{"cccc" : 123.22}');"""
- sql """insert into ${table_name} values (8, '{"cccc" : 123.11}');"""
- sql """insert into ${table_name} values (9, '{"ccccc" : [123]}');"""
- sql """insert into ${table_name} values (10, '{"ccccc" :
[123456789]}');"""
- sql """insert into ${table_name} values (11, '{"b" :
1111111111111111}');"""
- sql """insert into ${table_name} values (12, '{"b" : 1.222222}');"""
- sql """insert into ${table_name} values (13, '{"bb" : 1}');"""
- sql """insert into ${table_name} values (14, '{"bb" :
214748364711}');"""
- sql """insert into ${table_name} values (15, '{"A" : 1}');"""
+ // Keep conflicting values in one tablet and one rowset so type
resolution is deterministic.
+ create_table.call(table_name, "DUPLICATE", "1", "true")
Review Comment:
This makes the Variant numeric-conflict case deterministic by collapsing it
to one tablet and one rowset with compaction disabled, but it also removes the
storage topology that can create the conflict. The new
`NumericConflictResolvedAsJsonbPreservesOriginalNumberText` test explicitly
cites different tablets yet only parses two rows into one in-memory
`ColumnVariant`; it never writes, merges, reads, or compacts rowsets. Please
retain this stable case and add deterministic cross-rowset/cross-tablet
coverage with explicitly waited compaction so number-text behavior remains
covered end to end.
##########
fe/fe-common/src/main/java/org/apache/doris/common/FractionalFormat.java:
##########
@@ -26,42 +27,80 @@
public class FractionalFormat {
/**
- * Get string of double/float value for cast to string and output to mysql.
+ * Get the shortest string that round-trips to the given float value.
*
- * @param value The double/float value.
- * @param precision precision
- * @param sciFormat format for string with scientific form.
+ * @param value The float value.
* @return string value.
*/
- public static String getFormatStringValue(double value, int precision,
String sciFormat) {
+ public static String getFormatStringValue(float value) {
+ if (Float.isNaN(value)) {
+ return "NaN";
+ }
+ if (Float.isInfinite(value)) {
+ return value > 0 ? "Infinity" : "-Infinity";
+ }
+ if (value == 0) {
+ return Float.floatToRawIntBits(value) < 0 ? "-0" : "0";
+ }
+ BigDecimal exactValue = new BigDecimal(value);
+ int bits = Float.floatToRawIntBits(value);
+ for (int precision = 1; precision < 9; precision++) {
+ BigDecimal candidate = exactValue.round(new MathContext(precision,
RoundingMode.HALF_EVEN));
+ if
(Float.floatToRawIntBits(Float.parseFloat(candidate.toString())) == bits) {
+ return format(candidate);
+ }
+ }
+ return format(exactValue.round(new MathContext(9,
RoundingMode.HALF_EVEN)));
+ }
+
+ /**
+ * Get the shortest string that round-trips to the given double value.
+ *
+ * @param value The double value.
+ * @return string value.
+ */
+ public static String getFormatStringValue(double value) {
if (Double.isNaN(value)) {
return "NaN";
}
if (Double.isInfinite(value)) {
return value > 0 ? "Infinity" : "-Infinity";
}
- if (Double.compare(value, 0.0) == 0) {
- return "0";
- }
- if (Double.compare(value, -0.0) == 0) {
- return "-0";
+ if (value == 0) {
+ return Double.doubleToRawLongBits(value) < 0 ? "-0" : "0";
}
- int expLower = -4;
- int exponent = (int) Math.floor(Math.log10(Math.abs(value)));
- if (exponent < precision && exponent >= expLower) {
- BigDecimal bd = new BigDecimal(value);
- bd = bd.setScale(precision - bd.precision() + bd.scale(),
RoundingMode.HALF_UP);
- String result = bd.toPlainString();
- if (result.contains(".")) {
- result = result.replaceAll("0+$", "");
- if (result.endsWith(".")) {
- result = result.substring(0, result.length() - 1);
- }
+ BigDecimal exactValue = new BigDecimal(value);
+ long bits = Double.doubleToRawLongBits(value);
+ for (int precision = 1; precision < 17; precision++) {
Review Comment:
This precision search runs for every floating-point cell materialized by
transactional and group-commit inserts (`InsertUtils` and `GroupCommitPlanner`
both reach this visitor). Typical finite doubles repeatedly round a large exact
`BigDecimal`, allocate a string, and reparse it up to 16 times. On the changed
class, 100k random finite doubles measured 1.803 s versus 0.377 s for the old
formatter (4.79x); an independent probe also reproduced a substantial slowdown.
That adds seconds to large VALUES batches before RPC work. Please use a
bounded-allocation shortest converter and add a high-cardinality performance
guard.
##########
be/src/exprs/function/cast/cast_to_string.h:
##########
@@ -114,13 +113,7 @@ struct CastToString {
end = buffer + neg_inf_str_len;
}
} else {
- if constexpr (std::is_same_v<T, float>) {
- 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);
- }
Review Comment:
**Gate the new spelling across rolling upgrades.** This changes scalar-cast
semantics immediately on each upgraded BE, but old and new builds both
advertise `be_exec_version = 10` and this path does not consult runtime/query
compatibility state. During the supported BE-first rollout, a distributed query
can format the same DOUBLE `8.8` as `8.800000000000001` on an old BE and `8.8`
on a new one; `GROUP BY CAST(d AS STRING)` then treats them as different keys.
Please gate the new spelling from the upgraded FE (as the existing
`new_version_unix_timestamp` pattern does), or introduce the appropriate
execution-version transition.
**Also update the remaining reachable baselines.** Once the new mode is
enabled, `query_p0/outfile/test_outfile.groovy` directly selects the generated
`8.8` row while its unchanged `.out` still expects `8.800000000000001`;
`export_p0/outfile/native/test_outfile_native.groovy` has the same stale
spelling in both result blocks. Please regenerate those baselines so the
conditional outfile suites do not fail.
##########
fe/fe-common/src/main/java/org/apache/doris/common/FractionalFormat.java:
##########
@@ -26,42 +27,80 @@
public class FractionalFormat {
/**
- * Get string of double/float value for cast to string and output to mysql.
+ * Get the shortest string that round-trips to the given float value.
*
- * @param value The double/float value.
- * @param precision precision
- * @param sciFormat format for string with scientific form.
+ * @param value The float value.
* @return string value.
*/
- public static String getFormatStringValue(double value, int precision,
String sciFormat) {
+ public static String getFormatStringValue(float value) {
+ if (Float.isNaN(value)) {
+ return "NaN";
+ }
+ if (Float.isInfinite(value)) {
+ return value > 0 ? "Infinity" : "-Infinity";
+ }
+ if (value == 0) {
+ return Float.floatToRawIntBits(value) < 0 ? "-0" : "0";
+ }
+ BigDecimal exactValue = new BigDecimal(value);
+ int bits = Float.floatToRawIntBits(value);
+ for (int precision = 1; precision < 9; precision++) {
+ BigDecimal candidate = exactValue.round(new MathContext(precision,
RoundingMode.HALF_EVEN));
+ if
(Float.floatToRawIntBits(Float.parseFloat(candidate.toString())) == bits) {
+ return format(candidate);
+ }
+ }
+ return format(exactValue.round(new MathContext(9,
RoundingMode.HALF_EVEN)));
+ }
+
+ /**
+ * Get the shortest string that round-trips to the given double value.
+ *
+ * @param value The double value.
+ * @return string value.
+ */
+ public static String getFormatStringValue(double value) {
if (Double.isNaN(value)) {
return "NaN";
}
if (Double.isInfinite(value)) {
return value > 0 ? "Infinity" : "-Infinity";
}
- if (Double.compare(value, 0.0) == 0) {
- return "0";
- }
- if (Double.compare(value, -0.0) == 0) {
- return "-0";
+ if (value == 0) {
+ return Double.doubleToRawLongBits(value) < 0 ? "-0" : "0";
}
- int expLower = -4;
- int exponent = (int) Math.floor(Math.log10(Math.abs(value)));
- if (exponent < precision && exponent >= expLower) {
- BigDecimal bd = new BigDecimal(value);
- bd = bd.setScale(precision - bd.precision() + bd.scale(),
RoundingMode.HALF_UP);
- String result = bd.toPlainString();
- if (result.contains(".")) {
- result = result.replaceAll("0+$", "");
- if (result.endsWith(".")) {
- result = result.substring(0, result.length() - 1);
- }
+ BigDecimal exactValue = new BigDecimal(value);
+ long bits = Double.doubleToRawLongBits(value);
Review Comment:
This precision search runs for every floating-point cell materialized by
transactional and group-commit inserts (`InsertUtils` and `GroupCommitPlanner`
both reach this visitor). Typical finite doubles repeatedly round a large exact
`BigDecimal`, allocate a string, and reparse it up to 16 times. On the changed
class, 100k random finite doubles measured 1.803 s versus 0.377 s for the old
formatter (4.79x); an independent probe also reproduced a substantial slowdown.
That adds seconds to large VALUES batches before RPC work. Please use a
bounded-allocation shortest converter and add a high-cardinality performance
guard.
--
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]
