[GitHub] [hive] scarlin-cloudera commented on a change in pull request #2027: HIVE-24817: A "not in" clause returns incorrect data when there is co…

[GitBox <gi...@apache.org>]

scarlin-cloudera commented on a change in pull request #2027:
URL: https://github.com/apache/hive/pull/2027#discussion_r589822395



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/type/TypeCheckProcFactory.java
##########
@@ -1007,17 +1001,12 @@ protected T getXpathOrFuncExprNodeDesc(ASTNode node,
             T columnDesc = children.get(0);
             T valueDesc = interpretNode(columnDesc, children.get(i));
             if (valueDesc == null) {
-              if (hasNullValue) {
-                // Skip if null value has already been added
-                continue;
-              }
-              TypeInfo targetType = exprFactory.getTypeInfo(columnDesc);
+              // Keep original
+              TypeInfo targetType = exprFactory.getTypeInfo(children.get(i));
               if (!expressions.containsKey(targetType)) {
                 expressions.put(targetType, columnDesc);
               }
-              T nullConst = exprFactory.createConstantExpr(targetType, null);
-              expressions.put(targetType, nullConst);
-              hasNullValue = true;
+              expressions.put(targetType, children.get(i));
             } else {

Review comment:
       So I'm not sure how to address your comments, I"m gonna address all of them here.  Lemme take a step back and tell you what I've already discussed with Jesus and the direction we were going.
   
   So we know we're losing some optimizations as you've noted.  Jesus felt that they weren't that big of a deal.  For instance, we'd lose the optimization of "tinyint_col in (2500000)".  Previously, we saw this changing to false since a tinyint col can never be that value, but that check won't be optimized out now.  I think that's the main one I saw with the tests.
   
   So for now, we'd like to avoid a bigger rewrite.  He also noted that this optimization should perhaps be more in the Calcite framework, which makes sense to me.
   
   You did have one other comment about an "if" statement not being hit.  I'm not sure I understand the bug that you're referring to.  It's a bit complicated to understand, but it seems ok to me?  Can you explain this further?
   Thanks again!
   




----------------------------------------------------------------
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