Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support unwrap date to timestamp with time zone #14945

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Support unwrap date to timestamp with time zone #14945

merged 2 commits into from
Nov 16, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Nov 8, 2022 •
edited
Loading

Description

Fixes #5798

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:

# General
* Improve performance of specific queries which compare table columns of type 
  `date` with `timestamp with time zone` literals. ({issue}`5798`)

@cla-bot cla-bot bot added the cla-signed label Nov 8, 2022
findepi
findepi reviewed Nov 14, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapCastInComparison.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapCastInComparison.java Outdated
testUnwrap(session, inputType, "NULL", inputPredicate, expectedPredicate);
}

private void testUnwrap(Session session, String inputType, String inputValue, String inputPredicate, String expectedPredicate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the UnwrapCast perspective, does the inputValue, part of VALUES, matter?

(if it does, we should stop using VALUES for this test, we would need a temporary table)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the input value affects it. I left TODO comment as it's a pre-existing issue and the change seems not simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to understand that. Do you know what happens here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was doing something wrong. I reconfirmed the input value doesn't affect it. Removed a new testUnwrap

core/trino-main/src/test/java/io/trino/sql/query/TestUnwrapCastInComparison.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/sql/query/TestUnwrapCastInComparison.java Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/date-timestamp branch 2 times, most recently from 855c346 to be81989 Compare November 15, 2022 06:39
@ebyhr ebyhr marked this pull request as ready for review November 15, 2022 06:43
@ebyhr
Copy link
Member Author

ebyhr commented Nov 15, 2022

Addressed comments.

findepi
findepi reviewed Nov 15, 2022
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except open be81989#r1021802073.
(Ideally, there is no new testUnwrap overload.)

core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapCastInComparison.java Outdated
testUnwrap(session, inputType, "NULL", inputPredicate, expectedPredicate);
}

private void testUnwrap(Session session, String inputType, String inputValue, String inputPredicate, String expectedPredicate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to understand that. Do you know what happens here?

@findepi
Copy link
Member

findepi commented Nov 16, 2022

Thanks. LGTM

@ebyhr ebyhr merged commit 5f9d7f2 into trinodb:master Nov 16, 2022
@ebyhr ebyhr deleted the ebi/date-timestamp branch November 16, 2022 23:09
@ebyhr ebyhr mentioned this pull request Nov 16, 2022
Closed
@github-actions github-actions bot added this to the 404 milestone Nov 16, 2022
@colebow colebow mentioned this pull request Nov 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi left review comments

Assignees
No one assigned
Labels
cla-signed
Milestone
405
Development

Successfully merging this pull request may close these issues.

Optimize date and timestamp(p) with time zone comparisons
2 participants
@ebyhr @findepi