Rewrite casts in between predicate by findinpath · Pull Request #14648 · trinodb/trino

findinpath · 2022-10-14T21:18:12Z

Description

Alternative version for #14452 built on top of the work performed on #12797

This change allows the engine to infer that, for instance, given t::timestamp(6)

cast(t as date) BETWEEN DATE '2022-01-01' AND DATE '2022-01-02'

can be rewritten as

 t BETWEEN TIMESTAMP '2022-01-01 00:00:00.000000' AND  TIMESTAMP '2022-01-02 23:59:59.999999'

The change applies for the temporal types:

date
timestamp
timestamp with time zone (up to precision 6)

as well as the other types which benefit of implementations for getPreviousValue(Object) and getNextValue(Object).

Range predicate 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 manifests lists which play a key role in partition pruning or Iceberg data files), as ranges allow for intersection tests, something that is hard
to do in a generic manner for ConnectorExpression.

This is a spin-off from #14390

Additional details

The details of the logic of the method io.trino.sql.planner.iterative.rule.UnwrapCastInComparison.Visitor#rewriteBetweenPredicate can be verified for correctness with the test io.trino.sql.planner.TestUnwrapCastInComparison#testBetween

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Main
* Improve partition and data pruning when comparing casts with ranges in `BETWEEN` predicate

findinpath · 2022-10-14T21:25:15Z

Current limitation

I stumbled while trying to add the methods:

getPreviousValue(Object)
getNextValue(Object)

to DateType.
I am assuming that checking the validity of a date needs joda-time expertise which is not available in trino-spi module.

As an alternative we could use custom operators for nextval, previousval included in DateOperators (and the other operators for other types as well) to cover this need.

core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapCastInComparison.java

core/trino-spi/src/main/java/io/trino/spi/type/LongTimestampWithTimeZoneType.java

martint · 2023-08-11T16:32:01Z

I assume this is a typo, but in the motivating example the rewrite should be:

 t >= TIMESTAMP '2022-01-01 00:00:00.000000' AND t < TIMESTAMP '2022-01-03 00:00:00.000000'

(Note the precision of the first timestamp)

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

findinpath · 2023-08-14T20:12:31Z

Rebasing on top of master to reuse #18588

Going to Draft mode until the comments from the PR are addressed.

findinpath · 2023-09-06T06:29:29Z

Failure on ci/test-other-modules

https://github.com/trinodb/trino/actions/runs/6088945758/job/16520680223?pr=14648

Error:  io.trino.plugin.google.sheets.TestGoogleSheets.init -- Time elapsed: 1.023 s <<< FAILURE!
com.google.api.client.googleapis.json.GoogleJsonResponseException: 
503 Service Unavailable
POST https://sheets.googleapis.com/v4/spreadsheets?fields=spreadsheetId
{
  "code" : 503,
  "errors" : [ {
    "domain" : "global",
    "message" : "The service is currently unavailable.",
    "reason" : "backendError"
  } ],
  "message" : "The service is currently unavailable.",
  "status" : "UNAVAILABLE"
}

findepi · 2023-09-06T10:21:17Z

I assume this is a typo, but in the motivating example the rewrite should be:
 t >= TIMESTAMP '2022-01-01 00:00:00.000000' AND t < TIMESTAMP '2022-01-03 00:00:00.000000'

I think it might be desirable to have <= on the right, as this allows keeping BETWEEN as the predicate.

mosabua · 2024-01-11T23:57:33Z

Is this still in progress @findinpath ?

findinpath · 2024-01-12T08:28:48Z

@mosabua I did try repeatedly to find a maintainer to accompany this PR without success.

@martint can you suggest somebody to accompany eventually this PR ?

martint · 2024-01-12T16:44:19Z

@findinpath, sorry, this fell between the cracks. I'm happy to help get it in. Can you rebase?

findinpath · 2024-01-15T19:17:37Z

@martint I did rebase on top of the latest changes.
Please have a look.
Feel free to comment if the issue description is not making clear the purpose of this PR.

Heads'up: As a follow-up, i'll make a similar improvement for date_trunc (after reviving #14451)

findinpath · 2024-01-16T16:22:09Z

Unrelated CI failure #20391

github-actions · 2024-09-04T17:06:51Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-09-26T17:12:23Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

Co-authored-by: kabunchi <tal@varada.io>

This change allows the engine to infer that, for instance, given t::timestamp(6) cast(t as date) BETWEEN DATE '2022-01-01' AND DATE '2022-01-02' can be rewritten as t BETWEEN TIMESTAMP '2022-01-01 00:00:00' AND TIMESTAMP '2022-01-02 23:59:59.999999' Range predicate `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 manifests lists which play a key role in partition pruning or Iceberg data files), as ranges allow for intersection tests, something that is hard to do in a generic manner for `ConnectorExpression`.

kasiafi

Just started reviewing

kasiafi · 2025-01-29T12:22:18Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+            Expression trueIfNotNullExpression = trueIfNotNull(cast.expression());
+            if (trueIfNotNullExpression.equals(lowBoundUnwrapped)) {
+                return highBoundUnwrapped;
+            }
+            if (trueIfNotNullExpression.equals(highBoundUnwrapped)) {
+                return lowBoundUnwrapped;
+            }


Not that this is potentially incorrect. Consider

lowBoundUnwrapped: trueIfNotNull(cast)

highBoundUnwrapped: evaluates to true for null value of cast.

I understand this is not the case today because the lowBoundUnwrapped and highBoundUnwrapped can only take a few different forms, as they are results of the unwrapCast method.

However, the unwrapCast method might evolve over time, and this logic might get out of sync.

I suggest that before we proceed to combining lowBoundUnwrapped and highBoundUnwrapped, we first assert that both expressions satisfy some structural requirements.

kasiafi · 2025-01-29T12:23:51Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+            Expression falseIfNotNullExpression = falseIfNotNull(cast.expression());
+            if (falseIfNotNullExpression.equals(lowBoundUnwrapped) || falseIfNotNullExpression.equals(highBoundUnwrapped)) {
+                if (falseIfNotNullExpression.equals(lowBoundUnwrapped) && falseIfNotNullExpression.equals(highBoundUnwrapped)) {
+                    return falseIfNotNullExpression;


Is each of these cases tested?

kasiafi · 2025-01-29T13:52:01Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                Type highBoundType = highBoundUnwrappedComparison.right().type();
+
+                if (cast.expression().equals(lowBoundUnwrappedComparison.left()) &&
+                        Objects.equals(sourceType, lowBoundType) &&


Comparing the types is redundant. In Comparison, both operands are of the same type.

kasiafi · 2025-01-29T13:52:10Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                if (cast.expression().equals(lowBoundUnwrappedComparison.left()) &&
+                        Objects.equals(sourceType, lowBoundType) &&
+                        cast.expression().equals(highBoundUnwrappedComparison.left()) &&
+                        Objects.equals(sourceType, highBoundType)) {


same as above

kasiafi · 2025-01-29T13:55:08Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                        // Try to reconstruct the BETWEEN predicate with the cast unwrapped
+                        if (!(lowBoundUnwrappedComparison.right() instanceof Constant(Type _, Object lowBoundValue))
+                                || !(highBoundUnwrappedComparison.right() instanceof Constant(Type _, Object highBoundValue))) {
+                            return expression;


Why not and(lowBoundUnwrappedComparison, highBoundUnwrappedComparison)?

kasiafi · 2025-01-29T14:01:06Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                            return expression;
+                        }
+
+                        int compareLowBoundValueAndHighBoundValue = compare(sourceType, lowBoundValue, highBoundValue);


The compare method would throw exception if lowBoundValue or highBoundValue was null.

Again, we are making implicit assumptions about what the unwrapCast method returns.

kasiafi · 2025-01-29T14:10:48Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                            if (compareNextLowAndHighBound > 0) {
+                                return falseIfNotNull(cast.expression());
+                            }
+                        }


Could we refactor the preceding code for low and high bounds of the created between expression so that we

compute the low and high bounds (either as the constants from the comparisons or the next/previous values)

compare them and return falseIfNotNull(cast.expression()) if low > high

create and return the between expression

?

kasiafi · 2025-01-29T14:14:04Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                    }
+                }
+
+                return and(lowBoundUnwrappedComparison, highBoundUnwrappedComparison);


Is each case covered in tests?

kasiafi · 2025-01-29T14:17:16Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java

+                return and(lowBoundUnwrappedComparison, highBoundUnwrappedComparison);
+            }
+
+            return expression;


The unwrapCast method might return constant FALSE. I don't think this case is covered. The result would be FALSE.

cla-bot bot added the cla-signed label Oct 14, 2022

findinpath mentioned this pull request Oct 14, 2022

Unwrap casts in BETWEEN predicate #14452

Closed

findinpath commented Oct 15, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapCastInComparison.java Outdated Show resolved Hide resolved

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from c833e20 to ee0351c Compare October 27, 2022 14:53

findinpath marked this pull request as ready for review October 27, 2022 16:41

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from ee0351c to 4855e69 Compare August 7, 2023 20:01

findinpath changed the title ~~Draft: Rewrite casts in between predicate v2~~ Rewrite casts in between predicate Aug 7, 2023

github-actions bot added the iceberg Iceberg connector label Aug 7, 2023

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from 4855e69 to 8438ecd Compare August 8, 2023 15:43

findinpath mentioned this pull request Aug 8, 2023

Fix getNextValue() to increase the number of milliseconds if needed #18588

Merged

findinpath self-assigned this Aug 8, 2023

findinpath commented Aug 11, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/type/LongTimestampWithTimeZoneType.java Outdated Show resolved Hide resolved

martint reviewed Aug 11, 2023

View reviewed changes

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

findinpath marked this pull request as draft August 14, 2023 20:12

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from 10d5999 to 63ffd74 Compare August 14, 2023 20:12

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from 63ffd74 to 5ed39aa Compare September 5, 2023 19:30

findinpath marked this pull request as ready for review September 5, 2023 19:33

findinpath requested a review from findepi September 6, 2023 06:29

findinpath mentioned this pull request Jan 12, 2024

Rewrite date_trunc in BETWEEN predicate #14451

Closed

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch 2 times, most recently from c4ba962 to 2da4d98 Compare January 12, 2024 20:42

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from 2da4d98 to f0a1ad5 Compare January 15, 2024 19:13

findinpath requested a review from martint January 15, 2024 19:13

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from f0a1ad5 to 1a5764e Compare January 16, 2024 14:25

github-actions bot added the stale label Sep 4, 2024

github-actions bot closed this Sep 26, 2024

findinpath reopened this Oct 4, 2024

findinpath added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Oct 4, 2024

findinpath and others added 2 commits December 27, 2024 16:53

Adjust unwrapCast method to not be dependent on ComparisonExpression

3627875

Co-authored-by: kabunchi <tal@varada.io>

findinpath force-pushed the rewrite-casts-in-between-predicate-v2 branch from 9db4049 to 61c4581 Compare December 27, 2024 23:22

findinpath added 2 commits December 28, 2024 00:54

empty

ca41012

empty

1408d24

findinpath requested review from kasiafi and raunaqmorarka and removed request for findepi December 29, 2024 07:39

kasiafi reviewed Jan 29, 2025

View reviewed changes

Conversation

findinpath commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional details

Release notes

Uh oh!

findinpath commented Oct 14, 2022

Uh oh!

Uh oh!

Uh oh!

martint commented Aug 11, 2023

Uh oh!

Uh oh!

findinpath commented Aug 14, 2023

Uh oh!

findinpath commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Sep 6, 2023

Uh oh!

mosabua commented Jan 11, 2024

Uh oh!

findinpath commented Jan 12, 2024

Uh oh!

martint commented Jan 12, 2024

Uh oh!

findinpath commented Jan 15, 2024

Uh oh!

findinpath commented Jan 16, 2024

Uh oh!

github-actions bot commented Sep 4, 2024

Uh oh!

github-actions bot commented Sep 26, 2024

Uh oh!

kasiafi left a comment

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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

findinpath commented Oct 14, 2022 •

edited

Loading

findinpath commented Sep 6, 2023 •

edited

Loading