Unwrap IN predicate for 'cast in' and for 'date trunc'#18138
Unwrap IN predicate for 'cast in' and for 'date trunc'#18138kabunchi wants to merge 4 commits intotrinodb:masterfrom kabunchi:unwrap-in2
Conversation
There was a problem hiding this comment.
Instead of creating a comparison expression, change unwrapCast to take the three components of a comparison expression (operation, left, right) and call it directly: unwrapCast(EQUAL, value, rightExpression)
There was a problem hiding this comment.
implemented it in a different commit, had to change the return type of unwrapCast to Optional<Expression> because in this code I need to know if the unwrap happened or not, so it turned out to be more touched lines than I thought...
take a look.
had to rebase to get the already merged changes to UnwrapYear... aligned with the proposed change
There was a problem hiding this comment.
What's reverseOrder and why do we need it?
There was a problem hiding this comment.
as you can see below, when using IN predicate the output is in reverse order relatively to the input and the assertPlan does not handle it cleanly.
instead of IN() OR rand we get rand OR IN() which is equivalent logically of course.
As I assumed it's not a faulty behavior of the engine I thought this was the simplest solution, happy to consider other options.
There was a problem hiding this comment.
The usage of that specific query and predicate is an internal implementation detail of the testUnwrap function. If that's not working in general, we need to come up with a different way to test that an expression is being unwrapped properly.
Regarding the expression being reordered by the engine, we should look into why that is happening. In general, we try to preserve the order unless we can determine that one expression is cheaper to evaluate first (they currently get evaluated left to right)
Can you please give this PR a title? |
|
|
||
| // Simplify `year(d) ? value` | ||
| private Expression unwrapYear(ComparisonExpression expression) | ||
| private Optional<Expression> unwrapYear(ComparisonExpression.Operator operator, Expression leftExpression, Expression rightExpression) |
There was a problem hiding this comment.
There was a problem hiding this comment.
it will be in a different commit of course.
not sure a separate PR is needed.
kabunchi
changed the title
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
👋 @kabunchi @martint - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
4 participants
Description
Additional context and related issues
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.
( ) Release notes are required, with the following suggested text: