[GitHub] [hudi] beyond1920 commented on a diff in pull request #7801: [HUDI-5657] Fix NPE if filters condition contains null literal when using column stats data skipping for flink

2023-02-06 Thread via GitHub 


beyond1920 commented on code in PR #7801:
URL: https://github.com/apache/hudi/pull/7801#discussion_r1097337920


##
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/source/stats/TestExpressionEvaluator.java:
##
@@ -103,8 +108,9 @@ void testEqualTo() {
 equalTo.bindColStats(indexRow6, queryFields(2), rExpr);
 assertFalse(equalTo.eval(), "12 <> null");
 
-equalTo.bindVal(new ValueLiteralExpression(null, DataTypes.INT()));
-assertFalse(equalTo.eval(), "null <> null");
+assertThrows(
+IllegalStateException.class,

Review Comment:
   Make sense. 
   That's what I've done in the first commit, which is removed in the second 
commit.
   I revert this part now.



-- 
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: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hudi] beyond1920 commented on a diff in pull request #7801: [HUDI-5657] Fix NPE if filters condition contains null literal when using column stats data skipping for flink

2023-02-03 Thread via GitHub 


beyond1920 commented on code in PR #7801:
URL: https://github.com/apache/hudi/pull/7801#discussion_r1095512397


##
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/source/stats/TestExpressionEvaluator.java:
##
@@ -103,8 +108,9 @@ void testEqualTo() {
 equalTo.bindColStats(indexRow6, queryFields(2), rExpr);
 assertFalse(equalTo.eval(), "12 <> null");
 
-equalTo.bindVal(new ValueLiteralExpression(null, DataTypes.INT()));
-assertFalse(equalTo.eval(), "null <> null");
+assertThrows(
+IllegalStateException.class,

Review Comment:
   There is a preprocessing in `bindCall` which would translate to 
`AlwaysFalse` evaluator instead of EqualsTo/NotEqualsTo/In...
   So there is no need for the other evaluator to support null val.



-- 
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: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hudi] beyond1920 commented on a diff in pull request #7801: [HUDI-5657] Fix NPE if filters condition contains null literal when using column stats data skipping for flink

2023-02-03 Thread via GitHub 


beyond1920 commented on code in PR #7801:
URL: https://github.com/apache/hudi/pull/7801#discussion_r1095512397


##
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/source/stats/TestExpressionEvaluator.java:
##
@@ -103,8 +108,9 @@ void testEqualTo() {
 equalTo.bindColStats(indexRow6, queryFields(2), rExpr);
 assertFalse(equalTo.eval(), "12 <> null");
 
-equalTo.bindVal(new ValueLiteralExpression(null, DataTypes.INT()));
-assertFalse(equalTo.eval(), "null <> null");
+assertThrows(
+IllegalStateException.class,

Review Comment:
   There is a preprocessing in `bindCall` which would translate to 
`AlwaysFalse` evaluator instead of EqualsTo/NotEqualsTo/In...
   So the other evaluator could not support null val.



-- 
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: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hudi] beyond1920 commented on a diff in pull request #7801: [HUDI-5657] Fix NPE if filters condition contains null literal when using column stats data skipping for flink

2023-02-01 Thread via GitHub 


beyond1920 commented on code in PR #7801:
URL: https://github.com/apache/hudi/pull/7801#discussion_r1093081559


##
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/stats/ExpressionEvaluator.java:
##
@@ -282,6 +286,9 @@ public static LessThan getInstance() {
 
 @Override
 public boolean eval() {
+  if (this.val == null) {
+return false;
+  }

Review Comment:
   Sure.
   I introduce a new evaluator to handle this.



-- 
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: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org