github-actions[bot] commented on code in PR #67500:
URL: https://github.com/apache/doris/pull/67500#discussion_r3926208664
##########
be/src/core/value/time_value.h:
##########
@@ -43,7 +43,8 @@ class TimeValue {
constexpr static int64_t ONE_HOUR_SECONDS = 60 * ONE_MINUTE_SECONDS;
constexpr static uint32_t MICROS_SCALE = 6;
constexpr static int64_t MAX_TIME = 838 * ONE_HOUR_MICROSECONDS + 59 *
ONE_MINUTE_MICROSECONDS +
- 59 * ONE_SECOND_MICROSECONDS; //
838:59:59.000000
+ 59 * ONE_SECOND_MICROSECONDS +
+ (ONE_SECOND_MICROSECONDS - 1); //
838:59:59.999999
Review Comment:
[P1] Preserve scale invariants when widening the TIME boundary
This widened constant is used both as the representable limit and as the
result of scale-agnostic saturation, but the consumers do not normalize to the
declared result scale. On this head:
- `CAST(CAST('838:59:59.999999' AS TIME(6)) AS TIME(0))` rounds and carries
in `cast_to_date.h:319-339` to raw `3020400000000` (`839:00:00`), beyond this
new maximum. Only a `DCHECK(TimeValue::valid(time))` catches it; release builds
store it, while formatting clamps it back to `838:59:59` and `TIME_TO_SEC`
exposes `3020400`.
- Integer `SEC_TO_TIME` returns TIMEV2(0), but overflow now stores raw
`3020399999999`. Widening that value to TIMEV2(6) exposes `.999999`, unlike a
normal scale-0 endpoint, and raw equality/grouping/hash semantics can
distinguish the identically rendered values. TIMEDIFF and TIME add/sub have the
same problem when their result scale is below 6.
This is distinct from the older extraction/folding threads: those
current-head paths are aligned at scale 6; these failures are in scale
reduction and lower-scale payloads. Please make rounding and saturation
scale-aware (or enforce the chosen overflow semantics before storage) and cover
both signs at scales 0-5 with raw equality and `TIME_TO_SEC` oracles.
##########
regression-test/data/correctness/test_time_function.out:
##########
@@ -75,7 +75,7 @@
838:59:59.000000
-- !select --
-838:59:59.000000
+838:59:59.999999
Review Comment:
[P1] Regenerate the other saturation goldens
The shared `limit_with_bound` change reaches existing overflow cases outside
this suite. The two TIMEDIFF rows in
`regression-test/data/nereids_function_p0/scalar_function/D.out:496,499` and
the TIME(6) ADD_TIME row in
`regression-test/data/query_p0/sql_functions/datetime_functions/test_date_function_v2.out:137`
still expect `.000000`, but both implementations now return `.999999`. Please
regenerate and include those results (in addition to the already-threaded
TIME_FORMAT output); otherwise those full suites fail.
--
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]
