Simplify predicates involving date_trunc by findepi · Pull Request #14011 · trinodb/trino

findepi · 2022-09-06T13:38:15Z

No description provided.

findepi · 2022-09-07T21:09:42Z

CI #12385

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java

...ino-main/src/main/java/io/trino/sql/planner/iterative/rule/ReplaceDateTruncInComparison.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java

losipiuk · 2022-09-12T06:09:31Z

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java

is that still preferable when it comes to CPU cycles when we do not end up pushing down the expression?

Good question. This i don't know.
Do you think I should not do this?

Well - I think there may be a tradoff. But I do not know the answer. My guts say it is better to be able to do pushdown that have slight (if any) performance degradataion. To know if there really is any negative impact of that we would need some micro benchmarking - maybe we have alreayd IDK.

Well - I think there may be a tradoff. But I do not know the answer. My guts say it is better to be able to do pushdown that have slight (if any) performance degradataion.

I am convinced about that as well.

I can try to handle this case is the comparison context only.

I can try to handle this case is the comparison context only.

Done. This is now removed from here, taken care of by UnwrapDateTruncInComparison.

losipiuk

Awesome

When using day, month, year partitioning transforms, it's natural to use `year(c)`, `date(c)` or `date_trunc(..., c)` when querying an individual partition. Add test coverage documenting current state of Iceberg pushdown for such predicates.

findepi · 2022-09-12T08:15:01Z

(just rebased, to resolve a conflict)

martint · 2022-09-12T17:46:20Z

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java

It's better to add this in a dedicated rule than keep piling on this class and turning it into a kitchen sink of function translations.

`date_trunc('day', a_date)` is a no-op and can be replaced with `a_date`.

Range predicates (ComparisonExpression, BetweenPredicate) can be transformed into a `TupleDomain` and thus help with predicate pushdown. Range-based `TupleDomain` representation is critical for connectors which have min/max-based metadata (like Iceberg data files and manifest lists), as ranges allow for intersection tests, something that is hard to do in a generic manner for `ConnectorExpression`.

E.g. support for TIMESTAMP or INTERVAL literals was missing.

Engine's `UnwrapDateTruncInComparison` helps with predicates involving `date_trunc` over `date` and `timestamp` types, but it cannot help with `timestamp with time zone`, due to how `date_trunc` operates on value's local time and values are compared by point-in-time. In Iceberg, however, we know that all `timestamp with time zone` values are in UTC zone. This allows us to derive value range (`Domain`) for cases where engine wouldn't be able to do this.

findinpath · 2022-09-14T12:50:35Z

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java

                        || argumentType instanceof TimestampWithTimeZoneType
                        || argumentType instanceof VarcharType) {
-                    // prefer `CAST(x as DATE)` to `date(x)`
+                    // prefer `CAST(x as DATE)` to `date(x)`, see e.g. UnwrapCastInComparison


cla-bot bot added the cla-signed label Sep 6, 2022

findepi force-pushed the findepi/date-trunc-pushdown branch 2 times, most recently from 49b384c to ae59bd0 Compare September 7, 2022 12:23

findepi marked this pull request as ready for review September 7, 2022 12:23

findepi requested review from alexjo2144, findinpath, hashhar, homar, losipiuk and martint September 7, 2022 12:24

findepi added enhancement New feature or request performance labels Sep 7, 2022

findepi self-assigned this Sep 7, 2022

findepi mentioned this pull request Sep 7, 2022

Add query clause information to function analysis #13885

Closed

homar approved these changes Sep 7, 2022

View reviewed changes

findinpath reviewed Sep 8, 2022

View reviewed changes

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved

findinpath reviewed Sep 8, 2022

View reviewed changes

...o-main/src/main/java/io/trino/sql/planner/iterative/rule/CanonicalizeExpressionRewriter.java Outdated Show resolved Hide resolved

findinpath approved these changes Sep 9, 2022

View reviewed changes

findepi force-pushed the findepi/date-trunc-pushdown branch from 2449231 to 983998e Compare September 9, 2022 14:22

findepi requested review from findinpath and homar September 9, 2022 14:22

findepi mentioned this pull request Sep 9, 2022

Iceberg predicate pushdown for date/time columns and year() function #14078

Closed

findepi requested review from hashhar, losipiuk and martint and removed request for alexjo2144, hashhar, losipiuk and martint September 9, 2022 14:32

findepi requested a review from alexjo2144 September 9, 2022 14:33

losipiuk reviewed Sep 12, 2022

View reviewed changes

losipiuk approved these changes Sep 12, 2022

View reviewed changes

findepi added 2 commits September 12, 2022 10:02

Improve explanatory comment about date() vs CAST preference

70eeda6

findepi force-pushed the findepi/date-trunc-pushdown branch from 983998e to f274215 Compare September 12, 2022 08:14

findepi force-pushed the findepi/date-trunc-pushdown branch from f274215 to d2ade35 Compare September 12, 2022 08:25

homar approved these changes Sep 12, 2022

View reviewed changes

martint reviewed Sep 12, 2022

View reviewed changes

findepi added 6 commits September 13, 2022 21:16

Simplify no-op date_trunc on a date

9bb6d03

`date_trunc('day', a_date)` is a no-op and can be replaced with `a_date`.

Translate all literal types to connector expressions

a1ecd81

E.g. support for TIMESTAMP or INTERVAL literals was missing.

Use enhanced instanceof

320343e

Convert ConstraintExtractor.ExtractionResult to a record

8367d0a

findepi force-pushed the findepi/date-trunc-pushdown branch from d2ade35 to 92706fa Compare September 13, 2022 19:17

empty

5e3832a

losipiuk approved these changes Sep 14, 2022

View reviewed changes

findepi merged commit 154223c into trinodb:master Sep 14, 2022

findepi deleted the findepi/date-trunc-pushdown branch September 14, 2022 11:57

github-actions bot added this to the 396 milestone Sep 14, 2022

findepi mentioned this pull request Sep 14, 2022

Release notes for 396 #14047

Closed

findinpath reviewed Sep 14, 2022

View reviewed changes

colebow mentioned this pull request Sep 14, 2022

Add Trino 396 release notes #14109

Merged

This was referenced Sep 16, 2022

Simplify predicates involving date_trunc('hour') #14161

Merged

Fix scope omission in 396 release notes #14182

Merged

hantangwangd mentioned this pull request Mar 20, 2024

Unwrap functions on parameter of DataType/TimestampType used in comparison with a literal prestodb/presto#22244

Open

Conversation

findepi commented Sep 6, 2022

Uh oh!

findepi commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants