[fix](fe) Align FE constant folding results with execution#65319
[fix](fe) Align FE constant folding results with execution#65319morrySnow wants to merge 1 commit into
Conversation
Problem Summary: FE constant folding returned results that differed from execution for several Nereids expressions. DATE and DATEV2 casts parsed the whole datetime value and could round a valid max-date input past the DATETIMEV2 boundary before truncating to date. TIME values lost their sign when coerced to DATETIMEV2 for TIMEDIFF folding, and TIMEDIFF had no FE executable implementation. DecimalV3 zero values cast to string used BigDecimal scientific notation instead of the target scale. STR_TO_DATE only inferred DATE for literal format arguments, so constant dynamic formats such as CONCAT('%Y-', '%j') kept DATETIMEV2 schema. FIELD used Java NaN comparison, unlike execution where NaN matches NaN. This change aligns those FE folding paths with execution and adds focused unit coverage.
Fix FE constant folding results for several date/time, decimal, str_to_date, and NaN FIELD expressions.
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.rules.expression.FoldConstantTest
- Behavior changed: Yes. FE constant folding now matches execution for the covered expressions.
- Does this need documentation: No
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/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 found several FE constant-folding mismatches that can make folded results diverge from BE execution. I am requesting changes on the inline issues below.
Validation: static review only; FE test execution was not run because this checkout is missing thirdparty/installed/protoc.
| tz = "+08:00"; | ||
| } | ||
| if (!isDatetime) { | ||
| return getDateLiteralDateTime(year, month, date, hour, minute, second, targetType); |
There was a problem hiding this comment.
The date-only path returns here before the timezone suffix is validated. getDateLiteralDateTime(...) only checks hour/minute/second and discards tz, so inputs like 2024-05-01T00:00XYZ or 2024-05-01T00:00+14:30 now fold to a DATE/DATEV2 literal. BE cast execution still rejects those cases; the existing cast tests expect null for both invalid timezone names and +14:30. Please keep the timezone validation on this path before truncating to the date, for example by factoring the validation from getDateTimeLiteral(...) or passing tz into the date-only helper.
|
|
||
| @Override | ||
| protected Expression uncheckedCastTo(DataType targetType) throws AnalysisException { | ||
| long microsecondValue = ((Double) getValue()).longValue(); |
There was a problem hiding this comment.
This applies the signed full microsecond value before every target cast, but BE only uses the microsecond field for TIMEV2->DATETIMEV2. For DATE, DATEV2, and DATETIME(v1), cast_to_date.h adds signed hour/minute/second and ignores the fractional part. That means a fold such as cast(cast('-00:00:00.999999' as time(6)) as date) can move to the previous day in FE, while BE keeps the current date. Please keep the full microsecond path for DATETIMEV2 and mirror BE's hour/minute/second-only behavior for the other targets.
| } | ||
| this.negative = value < 0; | ||
| long v = (long) Math.abs(value); | ||
| long v = (long) Math.abs(Math.round(value)); |
There was a problem hiding this comment.
sec_to_time(DoubleLiteral) passes sec * 1000000 to this constructor, but BE does not round that value: TimeValue::from_double_with_limit truncates the double-derived microseconds. With Math.round, sec_to_time(cast(0.0000015 as double)) folds to 00:00:00.000002 in FE, while BE keeps 00:00:00.000001. Please preserve truncation for the sec_to_time(double) path, or otherwise split the rounding behavior so this constructor still matches BE execution.
| return (Literal) format; | ||
| } | ||
| if (format.isConstant()) { | ||
| Expression evaluated = ExpressionEvaluator.INSTANCE.eval(format); |
There was a problem hiding this comment.
This still does not handle nested constant format expressions. ExpressionEvaluator.eval(format) is shallow here, so a constant format like concat('%Y-', cast('%j' as string)) still contains a Cast child during signature inference and evaluates back to the original expression instead of a StringLikeLiteral. For str_to_date(col, concat('%Y-', cast('%j' as string))), the first argument is not constant, so the root call cannot later fold to a literal and callers can still see DATETIMEV2 even though the format has no time fields. Please recursively fold/normalize the format before deriving the signature, and add a nonconstant-input test for this case.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
2 participants
What problem does this PR solve?
Problem Summary: FE constant folding returned results that differed from execution for several Nereids expressions. DATE and DATEV2 casts parsed the whole datetime value and could round a valid max-date input past the DATETIMEV2 boundary before truncating to date. TIME values lost their sign when coerced to DATETIMEV2 for TIMEDIFF folding, and TIMEDIFF had no FE executable implementation. DecimalV3 zero values cast to string used BigDecimal scientific notation instead of the target scale. STR_TO_DATE only inferred DATE for literal format arguments, so constant dynamic formats such as CONCAT('%Y-', '%j') kept DATETIMEV2 schema. FIELD used Java NaN comparison, unlike execution where NaN matches NaN. This change aligns those FE folding paths with execution and adds focused unit coverage.
Release note
Fix FE constant folding results for several date/time, decimal, str_to_date, and NaN FIELD expressions.
Check List (For Author)