[fix](time) Fix TIMEV2 boundary validation - #67500
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: TIME(6) values at 838:59:59.999999 were accepted by string literals but rejected by BE runtime casting because literal validity reused the microsecond-free saturation bound. This made folded and Lambda paths inconsistent. Separate the representable boundary from the saturation boundary and align Nereids numeric literal validation, so both paths retain the boundary while protocol formatting and sec_to_time saturation remain compatible.
### Release note
Fix TIME(6) boundary values becoming NULL in constant-folding and Lambda expressions.
### Check List (For Author)
- Test: Regression test and Unit Test
- BE cast/time-bound unit tests and correctness/test_timev2_fold regression test
- Behavior changed: Yes. TIME(6) boundary values are valid consistently across folded and runtime paths.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review completed in 2 rounds on head 93e7c4dfaa3c58ead30b92ef9c0d9d687c6d9ba1. Three blocking correctness/test issues remain; see the inline comments.
Critical checkpoints
- Goal and proof: the new BE/FE bounds do admit the raw positive and negative TIME(6) boundary, and the unit tests cover that raw acceptance. The end-to-end goal is not yet met because semantic consumers and FE folding still disagree with BE runtime.
- Scope and parallel paths: the patch is small, but the shared validation/literal bounds affect strict and non-strict parsing, component extraction, TIME scale/date casts, TIME_FORMAT, result serialization, and executable constant folding. Those paths were traced; the three inline issues are the remaining blockers.
- Concurrency, lifecycle/static initialization, configuration, persistence/data writes, observability, and new FE-BE variables: not involved.
- Compatibility: there is no wire/storage layout change, and the old protocol/saturation ceiling is intentionally retained. However, FE/BE execution parity is currently broken for the newly admitted values.
- Tests/results: the added regression catches the former NULL in folded/runtime/lambda paths, but protocol rendering as
.000000masks raw semantic mismatches, and an existing TIME_FORMAT generated result is now stale. Per the review-runner instruction, no builds or tests were run locally. - Performance and memory safety: the constexpr bound change adds no meaningful hot-path or allocation concern.
User focus: no additional review focus was provided.
|
|
||
| static bool valid(double time) { return time <= MAX_TIME && time >= -MAX_TIME; } | ||
| static bool valid(double time) { | ||
| return time <= MAX_TIME_WITH_MICROSECONDS && time >= -MAX_TIME_WITH_MICROSECONDS; |
There was a problem hiding this comment.
[P1] Keep semantic extraction consistent with the widened valid range
After this check starts accepting 838:59:59.999999, the BE stores raw 3020399999999, but TimeValue::microsecond() still passes it through the old limit_with_bound() ceiling and therefore returns 0. A non-folded microsecond(CAST('838:59:59.999999' AS TIME(6))) consequently disagrees with Nereids folding, which reads TimeV2Literal.getMicroSecond() and returns 999999; TIMEV2-to-DATETIMEV2 and lower-scale TIME casts use the same component helpers. Please separate component extraction for an already-valid TIMEV2 value from call sites that intentionally apply MySQL-bound saturation, and cover the folded/non-folded extraction path.
| {{std::string("5656.3000000009")}, std::string("00:56:56.300000")}, | ||
| {{std::string("5656.3000007001")}, std::string("00:56:56.300001")}, | ||
| {{std::string("12:34:56.123")}, std::string("12:34:56.123")}, | ||
| {{std::string("838:59:59.999999")}, std::string("838:59:59.999999")}, |
There was a problem hiding this comment.
[P1] Update the existing TIME_FORMAT regression for this newly valid cast
This success case also changes doc/sql-manual/sql-functions/doc_date_functions_test: that suite feeds the VARCHAR values +/-838:59:59.999999 to TIME_FORMAT, whose signature implicitly casts them to TIMEV2. Its checked-in .out currently expects every formatted field for both rows to be NULL, but this parser/validation change makes the cast succeed and FunctionTimeFormat<TYPE_TIMEV2> return non-NULL strings. Please regenerate and include that existing result (and verify the intended fractional-field semantics), otherwise the full regression suite will fail.
| private static final TimeV2Literal MIN_VALUE = new TimeV2Literal(838, 59, 59, 0, 6, true); | ||
| private static final TimeV2Literal MAX_VALUE = new TimeV2Literal(838, 59, 59, 0, 6, false); | ||
| private static final TimeV2Literal MIN_VALUE = new TimeV2Literal(838, 59, 59, 999999, 6, true); | ||
| private static final TimeV2Literal MAX_VALUE = new TimeV2Literal(838, 59, 59, 999999, 6, false); |
There was a problem hiding this comment.
[P1] Preserve sec_to_time saturation during FE folding
This wider bound also changes the FE executable sec_to_time(DoubleLiteral), which directly constructs new TimeV2Literal(sec * 1000000). For sec_to_time(3020399.999999) FE folding now retains raw 838:59:59.999999, while BE runtime still calls TimeValue::from_double_with_limit() and deliberately saturates to 838:59:59.000000. Therefore comparing the result with CAST('838:59:59.999999' AS TIME(6)) folds to true but evaluates to false with constant folding skipped; the current protocol-rendered tests show .000000 for both and mask this. Please clamp the FE sec_to_time result to the saturation bound (or align the BE contract) and add folded/non-folded semantic coverage.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: TIMEV2 validation allowed 838:59:59.999999, but backend range limiting and result serialization used 838:59:59.000000. This made runtime CAST, SEC_TO_TIME, MAKETIME, and Lambda paths lose microseconds or disagree with FE constant folding. Unify the backend TIMEV2 bound at microsecond precision and preserve it in text and binary result serialization.
### Release note
TIME(6) boundary values now consistently preserve microseconds across constant folding, runtime evaluation, Lambda functions, SEC_TO_TIME, MAKETIME, and result serialization.
### Check List (For Author)
- Test: Regression test and Unit Test
- Regression test: correctness/test_timev2_fold
- Regression test: correctness/test_time_function
- Unit Test: TimeValueTest.time_bounds, MysqlRowBufferTest.TestBinaryTimeCompressedEncoding, and TIME cast tests
- Behavior changed: Yes. TIMEV2 values saturate and serialize at 838:59:59.999999 instead of dropping fractional seconds.
- Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Request changes. The direct TIMEV2(6) endpoint work is mostly consistent, but the shared-bound change leaves two blocking areas on the current head.
Blocking findings:
- The widened constant is used for both representability and scale-agnostic saturation. Lower-scale rounding can now store raw +/-839:00:00, and lower-scale saturated results can carry hidden six-digit precision. These are combined in one inline comment because they require the same scale-aware invariant.
- The widened saturation result changes three existing TIMEDIFF/ADD_TIME golden rows that this PR does not update.
- The already-open TIME_FORMAT golden-result thread remains valid and is not duplicated here. The older extraction and FE/BE scale-6 folding concerns are superseded by the current head.
Checkpoint conclusions:
- Goal, scope, conditions, and parallel paths: direct positive/negative TIMEV2(6) parsing, component extraction, FE/BE literal transport, and valid MySQL binary encoding agree at the new endpoint. Exact, just-inside, just-outside, both signs, scales 0-6, strict/non-strict casts, formatting, extraction, SEC_TO_TIME, MAKETIME, TIMEDIFF, and TIME add/sub paths were checked. The shared constant reaches more paths than the direct endpoint goal, and the lower-scale cast/saturation paths are not correct.
- Error handling: the scale-reduction overflow is guarded only by DCHECK, producing debug/release divergence and allowing an invalid raw value in release builds.
- Tests and results: the added tests establish rendered scale-6 behavior but do not exercise raw lower-scale equality/carry behavior. Additional checked-in saturation outputs are stale, in addition to the already-threaded TIME_FORMAT output. No local build or test was run because the automated-review prompt prohibits builds; CI style, secrets, license, and Cloud UT checks were green when inspected, while BE/FE UT and compile checks were still pending.
- Compatibility and serialization: there is no new storage or wire shape, and valid binary TIME encoding remains structurally correct. The broad saturation-contract concern is already covered by an existing thread and was not duplicated.
- Concurrency, lifecycle, configuration, persistence, transactions/data writes, observability, performance, and allocation: no applicable issue was found.
- User focus: no additional review focus was provided; the full PR was reviewed.
The review converged in Round 2: every candidate was independently verified and then accepted, dismissed with evidence, or duplicate-fenced against the live threads.
| 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 |
There was a problem hiding this 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 incast_to_date.h:319-339to raw3020400000000(839:00:00), beyond this new maximum. Only aDCHECK(TimeValue::valid(time))catches it; release builds store it, while formatting clamps it back to838:59:59andTIME_TO_SECexposes3020400.- Integer
SEC_TO_TIMEreturns TIMEV2(0), but overflow now stores raw3020399999999. 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.
|
|
||
| -- !select -- | ||
| 838:59:59.000000 | ||
| 838:59:59.999999 |
There was a problem hiding this 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.
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: TIME(6) values at 838:59:59.999999 were accepted by string literals but rejected by BE runtime casting because literal validity reused the microsecond-free saturation bound. This made folded and Lambda paths inconsistent. Separate the representable boundary from the saturation boundary and align Nereids numeric literal validation, so both paths retain the boundary while protocol formatting and sec_to_time saturation remain compatible.
Release note
Fix TIME(6) boundary values becoming NULL in constant-folding and Lambda expressions.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)