zabetak commented on code in PR #6082:
URL: https://github.com/apache/hive/pull/6082#discussion_r2367912580
##########
ql/src/test/queries/clientpositive/pointlookup6.q:
##########
@@ -17,3 +17,12 @@ WHERE r_table.string_col = l_table.string_col AND
l_table.string_col IN ('AAA111
SELECT l_table.string_col from l_table, r_table
WHERE r_table.string_col = l_table.string_col AND l_table.string_col IN
('AAA111', 'BBB222') AND r_table.string_col IN ('AAA111', 'BBB222');
+
+explain cbo
+SELECT * FROM r_table
+WHERE ( (
+ MINUTE(string_col) = 2 OR
+ MINUTE(string_col) = 10
+ )
+ OR (MINUTE(string_col) IS NULL)
+ );
Review Comment:
Is the additional OR nesting important for the repro? If not I would remove
it to avoid any potential confusion. There is also `set
hive.optimize.point.lookup.min=2;` in this file which adds some nuances with
respect to if the rule is firing or not in this case.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java:
##########
@@ -659,14 +658,17 @@ private static RexNode handleAND(RexBuilder rexBuilder,
RexCall call) {
} else {
inLHSExprToRHSExprs.put(c.exprNode, new
SimilarRexNodeElement(c.constNode));
}
- operands.remove(i);
- --i;
+ ++eqExpressionCount;
+ } else {
+ newOperands.add(operand);
}
}
+ if (inExpressionCount + eqExpressionCount ==
inLHSExprToRHSExprs.keySet().size()) {
Review Comment:
Is this if check equivalent to: `if (visitedRefs.size() ==
operands.size())`? Checking to see if we need all those new counters and
bookeeping.
##########
ql/src/test/queries/clientpositive/pointlookup6.q:
##########
@@ -17,3 +17,12 @@ WHERE r_table.string_col = l_table.string_col AND
l_table.string_col IN ('AAA111
SELECT l_table.string_col from l_table, r_table
WHERE r_table.string_col = l_table.string_col AND l_table.string_col IN
('AAA111', 'BBB222') AND r_table.string_col IN ('AAA111', 'BBB222');
+
+explain cbo
+SELECT * FROM r_table
+WHERE ( (
+ MINUTE(string_col) = 2 OR
+ MINUTE(string_col) = 10
+ )
+ OR (MINUTE(string_col) IS NULL)
+ );
Review Comment:
Additionally, if casts and timestamps are not important for the repro I
would simplify the query even more (e.g., use `UPPER(string_col)`).
##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestHivePointLookupOptimizerRule.java:
##########
@@ -520,4 +521,27 @@ public void testSameDecimalLiteralDifferentPrecision() {
System.out.println(condition);
assertEquals("IN($1, 10000:DECIMAL(19, 5), 11000:DECIMAL(19, 5))",
condition.toString());
}
+
+ @Test
+ public void testNothingToBeMergedInOrExpressionAndOperandOrderIsUnchanged() {
Review Comment:
Since the PR touches also the handleAND method to introduce bail out code
there should be tests covering that part as well.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java:
##########
@@ -659,14 +658,17 @@ private static RexNode handleAND(RexBuilder rexBuilder,
RexCall call) {
} else {
inLHSExprToRHSExprs.put(c.exprNode, new
SimilarRexNodeElement(c.constNode));
}
- operands.remove(i);
- --i;
+ ++eqExpressionCount;
+ } else {
+ newOperands.add(operand);
Review Comment:
Do we hit this `else` block when we skip an operand due to the
`isDeterminstic` check? I have the impression that the current version may
silently drop operands when non-deterministic functions are involved.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
