API: Add expression equivalence testing#4947
Conversation
a304dbd to
0ba6dd5
Compare
| static <T> Set<T> setOf(Iterable<Literal<T>> literals) { | ||
| Literal<T> lit = Iterables.get(literals, 0); | ||
| if (lit instanceof Literals.StringLiteral && lit.value() instanceof CharSequence) { | ||
| if (lit instanceof Literals.StringLiteral) { |
There was a problem hiding this comment.
This is always true because StringLiteral wraps a CharSequence.
| * @param other another expression | ||
| * @return true if the expressions are equivalent | ||
| */ | ||
| default boolean isEquivalentTo(Expression other) { |
There was a problem hiding this comment.
In which cases we use is prefix for boolean methods and in which no?
We are not very consistent in the code right now.
There was a problem hiding this comment.
I typically try to make the method name form a natural sentence in English, like if (someFilter.isEquivalentTo(other)) reads like "if some filter is equivalent to other". In some cases it makes more sense to use other words, like has.
I wasn't sure what to do with ExpressionUtil.equivalent since it compares two things, but areEquivalent(f1, f2, schema) doesn't feel right. I just left that as ExpressionUtil.equivalent. Suggestions are welcome!
| * @param spec a partition spec | ||
| * @return true if the expression will select whole partitions in the given spec | ||
| */ | ||
| public static boolean selectsPartitions(Expression expr, PartitionSpec spec) { |
There was a problem hiding this comment.
Am I correct this should allow us to optimize metadata-only deletes by handling obvious cases without the need to plan files with potential matches and invoking evaluators?
There was a problem hiding this comment.
Cool, I'll follow up with that once this is merged.
aokolnychyi
left a comment
There was a problem hiding this comment.
Looks great to me. One question about case sensitivity when building projections.
|
Thanks for reviewing, @aokolnychyi! |
|
@rdblue after I picked up this commit in master, my local build fails with Do you need to add something to .palantir/revapi.yml? |
This adds
ExpressionUtil.equivalentandExpressionUtil.selectsPartitionsto determine whether an expression will select whole files in a partition spec. This is needed to determine whether a filter is guaranteed to delete only whole files, or whether a row-level plan should be used. It is also needed to implement aggregate pushdown with non-trivial filters because filters must match whole files in order to use file metadata to satisfy a query.