initial implementation of startsWith #78
Conversation
| // due to the fact that startWith function in Expressions accepts only string types casting is a must here. | ||
| // in Unbound#bind function we added a check for that | ||
|
|
||
| return startsWith((BoundReference<String>) pred.ref(), pred.literal().to(pred.ref().type())); |
There was a problem hiding this comment.
Why is this calling to? A BoundReference guarantees that the type of its literal is correct and has already been coerced during the binding process.
| return new UnboundPredicate<>(Expression.Operation.NOT_EQ, ref(name), value); | ||
| } | ||
|
|
||
| public static <T> UnboundPredicate<T> startWith(String name, T value) { |
There was a problem hiding this comment.
Why does this accept T and not String? Shouldn't this always be a string, or does this also allow startsWith evaluation on byte arrays?
There was a problem hiding this comment.
Also, is it a typo that this is startWith and not startsWith?
There was a problem hiding this comment.
If I leave this as String it won't compile, I wanted to have it throw ValidationException as the other operations
you can see the test here
if I change it to string it won't compile
There was a problem hiding this comment.
What is the compile error? I'm surprised it won't compile if you remove the type variable and use String.
There was a problem hiding this comment.
I had a test checking that if I try to run startWith on anything other then string it will throw ValidationError which is the exception I throw in the bind method.
if startWith excepting only strings test would not compile.
I can remove this and change the type to String.
but does it mean that the bind method is useless for startWith?
There was a problem hiding this comment.
You can always use the Expressions.predicate method to create the predicate with a non-string value. I think that it's better to make Expressions.startsWith accept only a String so that the type system helps make expressions correct, not just binding checks.
api/src/main/java/com/netflix/iceberg/expressions/UnboundPredicate.java
Outdated
Show resolved
Hide resolved
api/src/test/java/com/netflix/iceberg/expressions/TestEvaluatior.java
Outdated
Show resolved
Hide resolved
|
@Liorba, thanks for working on this! This is a good start. To answer your questions:
This also needs to update the |
… into add-start-with-expression
|
@Liorba, thanks for updating, but this still needs to update the |
| evaluator.eval(TestHelpers.Row.of(("xyzffff")))); | ||
| } | ||
|
|
||
| @Test(expected = ValidationException.class) |
There was a problem hiding this comment.
This is more of a binding test, which should go in TestExpressionBinding.
|
@Liorba, if you want to continue working on this, please re-open it in the apache/incubator-iceberg repository. That's the project's new home. Thanks! |
2 participants
Fixes #49
Initial implementation only for iceberg-api expressions
Open issues: