-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Simplify predicates involving year function #15306
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
c7dd8ca to
206a236
Compare
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/ConstraintExtractor.java
Outdated
Show resolved
Hide resolved
206a236 to
43f5af3
Compare
|
Addressed comments. |
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
43f5af3 to
ada8cd3
Compare
|
CI hit #13633 |
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.
Am I correct it's not possible to normalize year() to date_trunc('year' or the other way around?
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.
If the value is known to be long (or int), let the caller do the conversion.
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.
.with(firstDayOfYear()) seems redundant
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.
Why not LocalDateTime.of(year, 1, 1, 0, 0)?
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.
LocalDateTime.of(year, 1, 1, 0, 0)?
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.
| if (argumentType != DATE && !(argumentType instanceof TimestampType)) { | |
| // e.g. year(INTERVAL) not handled here | |
| return expression; | |
| } |
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.
Not sure I understand the comment.
per the comment below (// if types do no match, this cannot be a comparison function) we didn't verify yet the call represents a comparison, so we don't know anything about relation between firstArgument and secondArgument
let's remove the 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.
LocalTime.MIN looks better in this context or perhaps
int year = toIntExact((Long) constant.getValue());
ZonedDateTime periodStart = ZonedDateTime.of(year, 1, 1, 0, 0, 0, 0, UTC);
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.
right-open ranges are easier to construct and work with, see unwrapTimestampTzToDateCast
in fact, there is high overlap between that method and the new one, opportunity for re-use
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.
The TODO got removed, but this still reads .isNotFullyPushedDown(); instead of .isFullyPushedDown().
It looks like the test asserts that the functionality doesn't work as intended.
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.
btw #16017
ada8cd3 to
474cb1c
Compare
474cb1c to
ae62119
Compare
|
I accidentally deleted the branch. Sent a new PR #16106 |
Description
Fixes #14078
Release notes
(x) Release notes are required, with the following suggested text: