API: one line fix in Expressions with tests #1722
Conversation
yyanyy
changed the title
|
Should we add a test since there isn't one? |
Thanks for the quick feedback! Looks like we have coverage for same method names that accepts string parameter in |
|
I mean it looks obviously correct to me :) I just hoped we could guard against future mistakes :) |
yyanyy
changed the title
| @@ -120,7 +120,7 @@ public static Expression not(Expression child) { | |||
| } | |||
|
|
|||
| public static <T> UnboundPredicate<T> notNull(UnboundTerm<T> expr) { | |||
| return new UnboundPredicate<>(Expression.Operation.IS_NULL, expr); | |||
| return new UnboundPredicate<>(Expression.Operation.NOT_NULL, expr); | |||
There was a problem hiding this comment.
I don't see any uses of this factory method in Spark, Flink, or MR/Hive so luckily, I don't think that this affects engines. (And I would expect it to be caught quickly by Spark tests if we did.) I think that decreases the urgency of this fix, but it still affects API users.
Thanks for catching this, @yyanyy!
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java
Outdated
Show resolved
Hide resolved
This reverts commit b538644.
|
Merged. Thanks for the quick fix, @yyanyy! |
Noticed this copy paste issue while updating
Expressions. Verified that no code is actually using it, so updating it shouldn't impact any test.