rdblue commented on a change in pull request #1722:
URL: https://github.com/apache/iceberg/pull/1722#discussion_r518911229
##########
File path: api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java
##########
@@ -63,11 +66,20 @@
@Test
public void testLessThan() {
- Evaluator evaluator = new Evaluator(STRUCT, lessThan("x", 7));
+ testLessThan(new Evaluator(STRUCT, lessThan("x", 7)),
+ new Evaluator(STRUCT, lessThan("s1.s2.s3.s4.i", 7)));
+ }
+
+ @Test
+ public void testLessThanWithUnboundTerm() {
+ testLessThan(new Evaluator(STRUCT, lessThan(self("x"), 7)),
+ new Evaluator(STRUCT, lessThan(self("s1.s2.s3.s4.i"), 7)));
Review comment:
Let's revert all of the changes to this file.
What we need to test for this PR is that `Expressions.notNull(UnboundTerm)`
creates an expression with operation `NOT_NULL`. The changes here are really
large and change many, many more cases.
These changes basically migrate to using the `UnboundTerm` factory methods
in `Expressions` for all of the cases, which is not what we want. This suite
tests the `Evaluator`, not `Expressions`.
It would be great to add more cases to the `TestExpressionHelpers` suite for
the `Expressions` factory methods that validates `notNull` results in
`UnboundPredicate` with `op() == NOT_NULL`. We can also add more cases to cover
more of `Expressions`, but I don't think this suite needs to change.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
