github-actions[bot] commented on code in PR #65609: URL: https://github.com/apache/doris/pull/65609#discussion_r3674376647
########## fe/fe-common/src/test/java/org/apache/doris/common/FractionalFormatTest.java: ########## @@ -0,0 +1,279 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.common; + +import com.fasterxml.jackson.core.io.schubfach.DoubleToDecimal; +import org.junit.Assert; +import org.junit.Test; + +import java.math.BigDecimal; +import java.math.MathContext; +import java.math.RoundingMode; +import java.text.NumberFormat; +import java.util.Random; + +public class FractionalFormatTest { + private static final int RANDOM_VALUES = 10_000_000; + private static final long RANDOM_SEED = 0xD0A15L; + private static volatile long blackHole; + + @Test + public void testBoundaryValues() { + Assert.assertEquals("0", FractionalFormat.getFormatStringValue(0.0)); + Assert.assertEquals("-0", FractionalFormat.getFormatStringValue(-0.0)); + Assert.assertEquals("NaN", FractionalFormat.getFormatStringValue(Double.NaN)); + Assert.assertEquals("Infinity", + FractionalFormat.getFormatStringValue(Double.POSITIVE_INFINITY)); + Assert.assertEquals("-Infinity", + FractionalFormat.getFormatStringValue(Double.NEGATIVE_INFINITY)); + Assert.assertEquals("0.0001", FractionalFormat.getFormatStringValue(1e-4)); + Assert.assertEquals("1e-05", FractionalFormat.getFormatStringValue(1e-5)); + Assert.assertEquals("1000000000000000", + FractionalFormat.getFormatStringValue(1e15)); + Assert.assertEquals("1e+16", FractionalFormat.getFormatStringValue(1e16)); + Assert.assertEquals("1e+23", FractionalFormat.getFormatStringValue(1e23)); + Assert.assertEquals("5.960464477539063e-08", + FractionalFormat.getFormatStringValue(Math.scalb(1.0, -24))); + Assert.assertEquals("5e-324", FractionalFormat.getFormatStringValue(Double.MIN_VALUE)); + Assert.assertEquals("1.7976931348623157e+308", + FractionalFormat.getFormatStringValue(Double.MAX_VALUE)); + + Assert.assertEquals("10000000", FractionalFormat.getFormatStringValue(1e7f)); + Assert.assertEquals("1.2621775e-29", + FractionalFormat.getFormatStringValue(Math.scalb(1.0f, -96))); + Assert.assertEquals("1e-45", FractionalFormat.getFormatStringValue(Float.MIN_VALUE)); + Assert.assertEquals("3.4028235e+38", + FractionalFormat.getFormatStringValue(Float.MAX_VALUE)); + } + + @Test + public void testRandomValuesRoundTrip() { + Random random = new Random(RANDOM_SEED); + for (int i = 0; i < 10_000; i++) { + double value = nextFiniteDouble(random); + String formatted = FractionalFormat.getFormatStringValue(value); + Assert.assertEquals(Double.doubleToRawLongBits(value), + Double.doubleToRawLongBits(Double.parseDouble(formatted))); + } + for (int i = 0; i < 10_000; i++) { + float value = nextFiniteFloat(random); + String formatted = FractionalFormat.getFormatStringValue(value); + Assert.assertEquals(Float.floatToRawIntBits(value), + Float.floatToRawIntBits(Float.parseFloat(formatted))); + } + } + + @Test + public void testHighCardinalityPerformance() { Review Comment: [P2] Keep the 10M-value benchmark out of the default FE unit suite This `@Test` allocates an ~80 MB array and performs five full 10M-value conversion passes—including the legacy BigDecimal loops—on every full FE UT run, but its only assertion is commented out and the results are not consumed. That adds recurring CI cost without detecting a regression, while dead-code optimization can also make the printed timings unreliable. Please move this experiment to JMH/manual benchmarking and retain only a small deterministic correctness test here. ########## regression-test/data/variant_p0/load.out: ########## @@ -212,7 +212,7 @@ [123] -- !sql_25 -- -50000 55000.00000000559 6150000 +50000 54999.99999999991 6150000 Review Comment: [P1] Stabilize this sum before recording a new baseline This change from `55000.00000000559` to `54999.99999999991` is not just the new formatter spelling the same value—the underlying DOUBLE sum changed. `create_table` randomly chooses 1–15 buckets on each run, so the partial-aggregation order for 50,000 copies of non-exact `1.1` is not stable; the doc-mode baseline now has `54999.99999999986`, and the suite comments record a third result. The new exact expectation can therefore flake again. Please preserve the randomized multi-tablet coverage but round this sum (or aggregate DECIMAL) before comparison. ########## regression-test/data/query_p0/sql_functions/test_template_one_arg.out: ########## @@ -14,11 +14,11 @@ \N \N -0.1001674211615598 --1.570796326794897 +-1.5707963267948966 0 0 0.1001674211615598 -1.570796326794897 +1.5707963267948966 Review Comment: [P1] Regenerate every reachable FLOAT/DOUBLE baseline This changed `asin(1)` demonstrates the new spelling, but the expected-result regeneration is repository-wide incomplete. Independently reachable unchanged examples include P0 math (48 old quarter-turn values), Hive page-index (4,566 old values), Kafka routine-load (29 old ARRAY<FLOAT> spellings across 3,696 occurrences), seven window outputs (59 old `0.07000000000000001` values), direct-MySQL output, P0 limit, MaxCompute write, Python-UDAF, Iceberg static-partition overwrite, analytic SQL, P2 scalar bitmap-date, and two-/three-level nested-S3 outputs. These are active comparisons over FLOAT/DOUBLE columns, not dead fixtures, so they deterministically mismatch after the formatter switch. Please regenerate every reachable FLOAT/DOUBLE result repository-wide, including the two outfile baselines already identified in `discussion_r3664689259`, and add a mechanical old-to-new token audit so additional suites are not missed. ########## 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: [P1] Handle persisted computed strings across this semantic change This formatter also defines stored data. Generated columns are always `STORED` and may participate in UNIQUE/AGGREGATE keys; for example, a valid generated key based on `CAST(CAST(a AS DOUBLE) + 0.8 AS STRING)` stores `8.800000000000001` before this change and `8.8` afterward for the same `a = 8`, leaving two physical keys. Synchronous MVs have the same durable boundary: `BindSink` computes their hidden columns for each new write, so a supported state such as `GROUP_CONCAT(CAST(kdbl + 0.8 AS VARCHAR))` can retain old spellings for historical rows and new spellings for later rows, making an MV rewrite disagree with recomputation from the base table. An execution-version gate only delays the transition; it rewrites neither stored keys nor MV state. Please define a compatibility/rebuild strategy (including recreating affected synchronous MVs) and add upgrade regressions for both paths. ########## regression-test/data/nereids_function_p0/scalar_function/Array1.out: ########## @@ -23137,7 +23137,7 @@ NaN \N \N 0E-56 -329042925341109538642.54940061644684653763274585062761879968415992086870360064 +329042925341109538642.5494006164468465376327458506276187996841599208687036006� Review Comment: [P1] Do not record an embedded NUL in this Decimal256 result This added expected row replaces the final `4` of the previous Decimal256 `array_sum` result with a literal NUL byte; the same corruption is repeated in the second active block at line 47112. `Array1.groovy` still executes `select array_sum(kadcml256) ...`, and this PR does not change the Decimal256/`array_sum` path, so this is not justified by the FLOAT/DOUBLE spelling update. A correct result will now mismatch this baseline; if the server really emitted the NUL, recording it would instead hide output corruption. Please restore/regenerate the value ending `...360064`, or fix the emitting path first if the byte is reproducible. -- 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]
