github-actions[bot] commented on code in PR #67500:
URL: https://github.com/apache/doris/pull/67500#discussion_r3925465605
##########
be/test/exprs/function/cast/cast_to_time_test.cpp:
##########
@@ -40,6 +40,8 @@ TEST_F(FunctionCastTest,
test_from_string_strict_mode_to_time) {
{{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")},
Review 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.
##########
be/src/core/value/time_value.h:
##########
@@ -151,7 +155,9 @@ class TimeValue {
return time;
}
- 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;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/TimeV2Literal.java:
##########
@@ -33,8 +33,8 @@
public class TimeV2Literal extends Literal {
private static final LocalDateTime START_OF_A_DAY = LocalDateTime.of(0, 1,
1, 0, 0, 0);
private static final LocalDateTime END_OF_A_DAY = LocalDateTime.of(9999,
12, 31, 23, 59, 59, 999999000);
- 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);
Review 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.
--
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]
