You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/09/13 01:24:54 UTC
[GitHub] [hive] jcamachor opened a new pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
jcamachor opened a new pull request #1492:
URL: https://github.com/apache/hive/pull/1492
…uses
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r487546716
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -734,6 +721,30 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
operands.remove(i);
--i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ String ref = c.exprNode.toString();
+ stringToExpr.put(ref, c.exprNode);
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ String expr = c.constNode.toString();
+ stringToExpr.put(expr, c.constNode);
Review comment:
Makes sense, I will simplify this code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r489581662
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
Review comment:
> isn't enough to move the logic to deal with the empty case to the end
of course - that could work as well! I just wanted to suggest that in case there is no "big benefit" keeping it - we may remove it as well - it contained correctness issues before this patch :)
> I prefer to avoid moving the rule at this stage and keep the change focused within the rule.
I only wanted to note that we should do that sometime later on
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #1492:
URL: https://github.com/apache/hive/pull/1492#issuecomment-692169075
@kgyrtkirk , could you take another look? Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor merged pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor merged pull request #1492:
URL: https://github.com/apache/hive/pull/1492
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r488774172
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -727,44 +708,58 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
} else {
for (int j = 1; j < inCall.getOperands().size(); j++) {
- String expr = inCall.getOperands().get(j).toString();
- inLHSExprToRHSExprs.put(ref, expr);
- stringToExpr.put(expr, inCall.getOperands().get(j));
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ }
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ RexNode ref = c.exprNode;
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ // Hence, since there are no common expressions and it is within an AND,
+ // we should return false
+ return rexBuilder.makeLiteral(false);
Review comment:
Good catch. It seems the problem was already there though. I am revisiting this logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #1492:
URL: https://github.com/apache/hive/pull/1492#issuecomment-700751767
@kgyrtkirk , addressed your last comment, let me know what you think of latest version. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r489412449
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
+ // We handle possible null values in the expressions.
+ List<RexNode> nullableExprs =
+ inLHSExprToRHSNullableExprs.get(ref)
+ .stream()
+ .map(n -> rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, ImmutableList.of(n)))
+ .collect(Collectors.toList());
+ return rexBuilder.makeCall(SqlStdOperatorTable.CASE,
Review comment:
I believe this would not need to be a `CASE` it could be an `AND` as well
```
(x IS NULL OR y IS NULL) AND null
```
but I know - it won't make much difference - the `CASE` might be more readbale :)
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
Review comment:
at this point - aren't we in the middle of processing all the operands of the AND?
```
x IN (1,2) AND x IN (3,4) AND y IN (1,2)
```
I suspect for the above expression we don't know anything about `y` (yet) - isn't that a problem?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
Review comment:
right now I don't fully agree with this logic - if we have an expression like:
```
x IN (1,2) AND x in (3,4) and y IN (5,6)
```
it's clear that we may only return `false` or `null`; but I think we may still need to choose `false` based on the other expressions; for the above:
* when `x is null, y=5` then the result will be `null`
* but when `x is null, y=1` then the result will be `false`
in light of this reasoning it seem to me that this starts looking more and more sophisticated....
I think it might worth a try to see if these "early-return-type" simplifications kick-in a lot or not - and as an alternative approach consider removing them instead of saving them.
I think at some point we should run the simplification earlier when the `OR`s are still open - so that it could analyze-it properly
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r488766066
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -727,44 +708,58 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
} else {
for (int j = 1; j < inCall.getOperands().size(); j++) {
- String expr = inCall.getOperands().get(j).toString();
- inLHSExprToRHSExprs.put(ref, expr);
- stringToExpr.put(expr, inCall.getOperands().get(j));
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ }
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ RexNode ref = c.exprNode;
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ // Hence, since there are no common expressions and it is within an AND,
+ // we should return false
+ return rexBuilder.makeLiteral(false);
Review comment:
I think this may not neccessarily false; consider:
```
x=1 and x=2
```
which is `not true`; but in case `x is null` - the result of the `AND` will be `null`
instead of false this could be only rewritten to `x is null and null`; but even in that case there could be other stuff which might affect null/false;
so I think we may not do a return here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r489718951
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
Review comment:
@kgyrtkirk I pushed a new commit and added a few more test cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r489501206
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
Review comment:
I agree. Isn't enough to move the logic to deal with the `empty` case to the end, instead of doing early bail out? I think that would fix the issue. I need to deal with the 'empty' case in some way.
I prefer to avoid moving the rule at this stage and keep the change focused within the rule. Otherwise, we may have to deal with closing and re-opening, plus the fix seems more risky (including backporting).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r495843137
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -757,10 +752,15 @@ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
}
}
// Create IN clauses
- final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ final List<RexNode> newOperands = createInClauses(rexBuilder,
+ visitedRefs, inLHSExprToRHSExprs, inLHSExprToRHSNullableExprs);
newOperands.addAll(operands);
// Return node
- return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ RexNode result = RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ if (!result.getType().equals(call.getType())) {
Review comment:
Is there any particular reason why do we need this? (I think it was not needed before)
I believe this will at most only add a cast to wieden nullability
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r487546646
##########
File path: ql/src/test/results/clientpositive/perf/tez/constraints/cbo_query74.q.out
##########
@@ -147,7 +147,7 @@ HiveSortLimit(sort0=[$2], sort1=[$0], sort2=[$1], dir0=[ASC], dir1=[ASC], dir2=[
HiveProject(ss_sold_date_sk=[$0], ss_customer_sk=[$3], ss_net_paid=[$20])
HiveTableScan(table=[[default, store_sales]], table:alias=[store_sales])
HiveProject(d_date_sk=[$0])
- HiveFilter(condition=[AND(=($1, 1999), IN($1, 1998, 1999))])
+ HiveFilter(condition=[=($1, 1999)])
Review comment:
What seems to happen is that we close these IN clauses before we call the join propagation rule, which leads to the inference of new predicates. Then the RexSimplify does not deal with the IN+EQUALS clauses and the HivePointLookup cannot deal with the EQUALS.
We could possibly change the rules order too but I was not brave enough to do that :) Plus I think supporting the degenerate case of EQUALS in HivePointLookup should be fine.
Iirc there is work going on in Calcite to finally have more extensive support for IN in RexSimplify, which you suggested some time ago... I hope that work goes in and we can simplify this whole code path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r496112152
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -757,10 +752,15 @@ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
}
}
// Create IN clauses
- final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ final List<RexNode> newOperands = createInClauses(rexBuilder,
+ visitedRefs, inLHSExprToRHSExprs, inLHSExprToRHSNullableExprs);
newOperands.addAll(operands);
// Return node
- return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ RexNode result = RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ if (!result.getType().equals(call.getType())) {
Review comment:
Once again, good catch. This compensation was coming from a previous version of the patch :(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #1492:
URL: https://github.com/apache/hive/pull/1492#issuecomment-691690227
Thanks for checking Zoltan! I had in draft because I wanted to regenerate the q files before making it final, but the code was ready to be reviewed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r489501206
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -678,100 +679,135 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
@Override public RexNode visitCall(RexCall call) {
- final RexNode node;
- final List<RexNode> operands;
- final List<RexNode> newOperands;
- final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
switch (call.getKind()) {
case AND:
- // IN clauses need to be combined by keeping only common elements
- operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- Set<RexNode> expressions = Sets.newHashSet();
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- expressions.add(inCall.getOperands().get(j));
- }
- inLHSExprToRHSExprs.get(ref).retainAll(expressions);
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- }
- operands.remove(i);
- --i;
- } else if (operand.getKind() == SqlKind.EQUALS) {
- Constraint c = Constraint.of(operand);
- if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
- continue;
+ return handleAND(rexBuilder, call);
+ case OR:
+ return handleOR(rexBuilder, call);
+ default:
+ return super.visitCall(call);
+ }
+ }
+
+ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping only common elements
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ // We will use this set to keep those expressions that may evaluate
+ // into a null value.
+ final Multimap<RexNode,RexNode> inLHSExprToRHSNullableExprs = LinkedHashMultimap.create();
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenAnd(call.getOperands()));
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ if (ref.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, ref);
+ }
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ Set<RexNode> expressions = Sets.newHashSet();
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ expressions.add(constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- RexNode ref = c.exprNode;
- if (inLHSExprToRHSExprs.containsKey(ref)) {
- inLHSExprToRHSExprs.get(ref).retainAll(Collections.singleton(c.constNode));
- if (!inLHSExprToRHSExprs.containsKey(ref)) {
- // Note that Multimap does not keep a key if all its values are removed.
- // Hence, since there are no common expressions and it is within an AND,
- // we should return false
- return rexBuilder.makeLiteral(false);
- }
- } else {
- inLHSExprToRHSExprs.put(ref, c.constNode);
+ }
+ inLHSExprToRHSExprs.get(ref).retainAll(expressions);
+ if (!inLHSExprToRHSExprs.containsKey(ref)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, ref, inLHSExprToRHSNullableExprs);
+ }
+ } else {
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ RexNode constNode = inCall.getOperands().get(j);
+ inLHSExprToRHSExprs.put(ref, constNode);
+ if (constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(ref, constNode);
}
- operands.remove(i);
- --i;
}
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeConjunction(rexBuilder, newOperands, false);
- break;
- case OR:
- // IN clauses need to be combined by keeping all elements
- operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
- for (int i = 0; i < operands.size(); i++) {
- RexNode operand = operands.get(i);
- if (operand.getKind() == SqlKind.IN) {
- RexCall inCall = (RexCall) operand;
- if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
- continue;
- }
- RexNode ref = inCall.getOperands().get(0);
- for (int j = 1; j < inCall.getOperands().size(); j++) {
- inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
- }
- operands.remove(i);
- --i;
+ operands.remove(i);
+ --i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ if (c.exprNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.exprNode);
+ }
+ if (c.constNode.getType().isNullable()) {
+ inLHSExprToRHSNullableExprs.put(c.exprNode, c.constNode);
+ }
+ if (inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ inLHSExprToRHSExprs.get(c.exprNode).retainAll(Collections.singleton(c.constNode));
+ if (!inLHSExprToRHSExprs.containsKey(c.exprNode)) {
+ // Note that Multimap does not keep a key if all its values are removed.
+ return createResultFromEmptySet(rexBuilder, c.exprNode, inLHSExprToRHSNullableExprs);
}
+ } else {
+ inLHSExprToRHSExprs.put(c.exprNode, c.constNode);
}
- // Create IN clauses
- newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
- newOperands.addAll(operands);
- // Return node
- node = RexUtil.composeDisjunction(rexBuilder, newOperands, false);
- break;
- default:
- return super.visitCall(call);
+ operands.remove(i);
+ --i;
+ }
}
- return node;
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode handleOR(RexBuilder rexBuilder, RexCall call) {
+ // IN clauses need to be combined by keeping all elements
+ final List<RexNode> operands = new ArrayList<>(RexUtil.flattenOr(call.getOperands()));
+ final Multimap<RexNode,RexNode> inLHSExprToRHSExprs = LinkedHashMultimap.create();
+ for (int i = 0; i < operands.size(); i++) {
+ RexNode operand = operands.get(i);
+ if (operand.getKind() == SqlKind.IN) {
+ RexCall inCall = (RexCall) operand;
+ if (!HiveCalciteUtil.isDeterministic(inCall.getOperands().get(0))) {
+ continue;
+ }
+ RexNode ref = inCall.getOperands().get(0);
+ for (int j = 1; j < inCall.getOperands().size(); j++) {
+ inLHSExprToRHSExprs.put(ref, inCall.getOperands().get(j));
+ }
+ operands.remove(i);
+ --i;
+ }
+ }
+ // Create IN clauses
+ final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ newOperands.addAll(operands);
+ // Return node
+ return RexUtil.composeDisjunction(rexBuilder, newOperands, false);
+ }
+
+ private static RexNode createResultFromEmptySet(RexBuilder rexBuilder,
+ RexNode ref, Multimap<RexNode, RexNode> inLHSExprToRHSNullableExprs) {
+ if (inLHSExprToRHSNullableExprs.containsKey(ref)) {
Review comment:
I agree. Isn't enough to move the logic to deal with the `empty` case to the end, instead of doing early bail out? I think that would fix the issue. I need to deal with the `empty` case in some way.
I prefer to avoid moving the rule at this stage and keep the change focused within the rule. Otherwise, we may have to deal with closing and re-opening, plus the fix seems more risky (including backporting).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r487545193
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -599,7 +587,7 @@ public ConstraintGroup(RexNode rexNode) {
if (constraint == null) {
throw new SemanticException("Unable to find constraint which was earlier added.");
}
- ret.add(constraint.exprNode);
+ ret.add(constraint.constNode);
Review comment:
I was also surprised we did not catch it before! But that's OK, it was working as expected and now they are swapped back!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r487511233
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -734,6 +721,30 @@ private RexNode useStructIfNeeded(List<? extends RexNode> columns) {
}
operands.remove(i);
--i;
+ } else if (operand.getKind() == SqlKind.EQUALS) {
+ Constraint c = Constraint.of(operand);
+ if (c == null || !HiveCalciteUtil.isDeterministic(c.exprNode)) {
+ continue;
+ }
+ String ref = c.exprNode.toString();
+ stringToExpr.put(ref, c.exprNode);
+ if (inLHSExprToRHSExprs.containsKey(ref)) {
+ String expr = c.constNode.toString();
+ stringToExpr.put(expr, c.constNode);
Review comment:
note: we have RexNode-s are comparable; this string2expr stuff is not neccessarily needed anymore
##########
File path: ql/src/test/results/clientpositive/perf/tez/constraints/cbo_query74.q.out
##########
@@ -147,7 +147,7 @@ HiveSortLimit(sort0=[$2], sort1=[$0], sort2=[$1], dir0=[ASC], dir1=[ASC], dir2=[
HiveProject(ss_sold_date_sk=[$0], ss_customer_sk=[$3], ss_net_paid=[$20])
HiveTableScan(table=[[default, store_sales]], table:alias=[store_sales])
HiveProject(d_date_sk=[$0])
- HiveFilter(condition=[AND(=($1, 1999), IN($1, 1998, 1999))])
+ HiveFilter(condition=[=($1, 1999)])
Review comment:
I see that this patch works - but would have thinked that we didn't need something like this anymore - because IN is opened in an early phase - so Calcite should see a bunch of ANDs and ORs - and if that's true - RexSimplify could make this simplification - and there would be no need to enhance PointLookup...
I wonder how the IN was retained ...or re-created...
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -599,7 +587,7 @@ public ConstraintGroup(RexNode rexNode) {
if (constraint == null) {
throw new SemanticException("Unable to find constraint which was earlier added.");
}
- ret.add(constraint.exprNode);
+ ret.add(constraint.constNode);
Review comment:
oh my; Constrain had it's constructor arguments swapped! what a typo!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1492: HIVE-24154: Missing simplification opportunity with IN and EQUALS cla…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1492:
URL: https://github.com/apache/hive/pull/1492#discussion_r496112152
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java
##########
@@ -757,10 +752,15 @@ private static RexNode handleAND(RexBuilder rexBuilder, RexCall call) {
}
}
// Create IN clauses
- final List<RexNode> newOperands = createInClauses(rexBuilder, inLHSExprToRHSExprs);
+ final List<RexNode> newOperands = createInClauses(rexBuilder,
+ visitedRefs, inLHSExprToRHSExprs, inLHSExprToRHSNullableExprs);
newOperands.addAll(operands);
// Return node
- return RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ RexNode result = RexUtil.composeConjunction(rexBuilder, newOperands, false);
+ if (!result.getType().equals(call.getType())) {
Review comment:
Once again, good catch. This compensation was coming from a previous version of the patch :( I have pushed a new commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org