Unwrap year in comparison also for IN predicate #18092
Conversation
| { | ||
| Expression value = node.getValue(); | ||
| Expression valueList = node.getValueList(); | ||
| Expression inPredicate = treeRewriter.defaultRewrite((Expression) node, null); |
There was a problem hiding this comment.
@ebyhr I am not sure I need this line. please advise.
There was a problem hiding this comment.
i think it would be better to default-rewrite (recursive) first, then to extract the attributes, just in case they got rewritten or simplified (don't think it matters currently, matter of style/future-proofing)
InPredicate inPredicate = treeRewriter.defaultRewrite(node, null);
Expression value = inPredicate.getValue();
Expression valueList = inPredicate.getValueList();
| @@ -124,6 +129,36 @@ public Expression rewriteComparisonExpression(ComparisonExpression node, Void co | |||
| return unwrapYear(expression); | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
we probably want similar change for UnwrapDateTruncInComparison and UnwrapCastInComparison?
| // Convert each value to a comparison expression and try to unwrap it. | ||
| // unwrap the InPredicate only in case we manage to unwrap the entire value list | ||
| List<Expression> expressionsList = inListExpression.getValues(); | ||
| ArrayList<Expression> comparisonExpressionsList = new ArrayList<>(expressionsList.size()); |
There was a problem hiding this comment.
use ImmutableList.Builder
(per style, but also to avoid copy when creating new LogicalExpression)
| { | ||
| Expression value = node.getValue(); | ||
| Expression valueList = node.getValueList(); | ||
| Expression inPredicate = treeRewriter.defaultRewrite((Expression) node, null); |
There was a problem hiding this comment.
i think it would be better to default-rewrite (recursive) first, then to extract the attributes, just in case they got rewritten or simplified (don't think it matters currently, matter of style/future-proofing)
InPredicate inPredicate = treeRewriter.defaultRewrite(node, null);
Expression value = inPredicate.getValue();
Expression valueList = inPredicate.getValueList();
please avoid mixing changes with rebase. it's hard to notice what chaned. |
| Expression value = node.getValue(); | ||
| Expression valueList = node.getValueList(); |
There was a problem hiding this comment.
| Expression value = node.getValue(); | |
| Expression valueList = node.getValueList(); | |
| Expression value = inPredicate.getValue(); | |
| Expression valueList = inPredicate.getValueList(); |
There was a problem hiding this comment.
done. I wasn't sure that the returned expression from defaultRewriter must be an InPredicate so I also added a validation.
| List<Expression> expressionsList = inListExpression.getValues(); | ||
| ImmutableList.Builder<Expression> comparisonExpressionsListBuilder = ImmutableList.builder(); | ||
| for (Expression currExpression : expressionsList) { |
There was a problem hiding this comment.
- avoid abbreviations ("curr")
comparisonExpressionsListBuilderis a long name and the fact it's listbuilder is known from its typeexpressionsListcan be inlined
| List<Expression> expressionsList = inListExpression.getValues(); | |
| ImmutableList.Builder<Expression> comparisonExpressionsListBuilder = ImmutableList.builder(); | |
| for (Expression currExpression : expressionsList) { | |
| ImmutableList.Builder<Expression> comparisonExpressions = ImmutableList.builderWithExpectedSize(inListExpression.getValues().size()); | |
| for (Expression rightExpression : inListExpression.getValues()) { | |
| ComparisonExpression comparisonExpression = new ComparisonExpression(EQUAL, value, rightExpression); |
core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapYearInComparison.java
Outdated
Show resolved
Hide resolved
|
@kabunchi thanks, will merge when the build passes |
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: