-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](Nereids) fix unix_timestamp #49686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 34199 ms |
TPC-DS: Total hot run time: 193400 ms |
ClickBench: Total hot run time: 31.88 s |
9bab166 to
377978a
Compare
|
run buildall |
TPC-H: Total hot run time: 34463 ms |
TPC-DS: Total hot run time: 188029 ms |
ClickBench: Total hot run time: 31.29 s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the unix_timestamp behavior in Nereids by adjusting the timezone conversion logic to correctly account for boundary conditions.
- Normalize the provided LocalDateTime to UTC upfront.
- Remove the redundant conversion in the duration calculation.
- Ensure the boundary checks against the corrected UTC value.
Files not reviewed (1)
- regression-test/suites/nereids_p0/expression/fold_constant/fold_constant_date_arithmatic.groovy: Language not supported
Comments suppressed due to low confidence (1)
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:577
- [nitpick] Consider replacing ZoneId.of('UTC+0') (used on line 578) with ZoneOffset.UTC to clarify intent and improve readability.
dateTime = dateTime.atZone(DateUtils.getTimeZone())
377978a to
8e3569e
Compare
|
run buildall |
|
PR approved by anyone and no changes requested. |
TPC-H: Total hot run time: 34262 ms |
TPC-DS: Total hot run time: 186172 ms |
ClickBench: Total hot run time: 30.76 s |
|
run p0 |
|
run cloud_p0 |
|
PR approved by at least one committer and no changes requested. |
zclllyybb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ary condition should changed (apache#49686)
…ary condition should changed (apache#49686)
…ary condition should changed (#49686)
…ary condition should changed (apache#49686)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
when considering timezone to unix_timestamp, the boundary condition should changed
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)