Add PredicateCompiler to SPI #12729
Add PredicateCompiler to SPI #12729
Conversation
mbasmanova
force-pushed
the
predicate-compiler
branch
3 times, most recently
from
49203be to
b781698
Compare
mbasmanova
changed the title
mbasmanova
requested review from
highker,
rongrong,
oerling,
tdcmeehan,
elonazoulay and
yingsu00
mbasmanova
force-pushed
the
predicate-compiler
branch
from
b781698 to
62dc9fe
Compare
presto-main/src/main/java/com/facebook/presto/sql/gen/RowExpressionPredicateCompiler.java
Outdated
Show resolved
Hide resolved
| requireNonNull(filter, "filter is null"); | ||
|
|
||
| PageFieldsToInputParametersRewriter.Result result = rewritePageFieldsToInputParameters(filter); | ||
| int[] inputChannels = result.getInputChannels().getInputChannels().stream() |
There was a problem hiding this comment.
nit
int[] inputChannels = result.getInputChannels()
.getInputChannels()
.stream()
.mapToInt(Integer::intValue)
.toArray();There was a problem hiding this comment.
I've been following the convention to put a line break after stream() and then put each transformation on a separate line.
| return filterCache.getUnchecked(predicate); | ||
| } | ||
|
|
||
| private Supplier<Predicate> compileFilterInternal(RowExpression filter, Optional<String> classNameSuffix) |
There was a problem hiding this comment.
compileFilterInternal(RowExpression predicate, Optional<String> classNameSuffix)
| import static io.airlift.bytecode.expression.BytecodeExpressions.not; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class RowExpressionPredicateCompiler |
There was a problem hiding this comment.
High-level comments on this class: There are many overlappings of this class with PageFunctionCompiler. But of course, there are some subtleties are different. Not sure if it worth extracting the common code path into utils. If not, can we add some basic tests like TestPageFunctionCompiler?
| }; | ||
| } | ||
|
|
||
| private static ParameterizedType generateFilterClassName(Optional<String> classNameSuffix) |
There was a problem hiding this comment.
We might need to switch all filters to predicates in this file.
| */ | ||
| package com.facebook.presto.spi.relation; | ||
|
|
||
| public interface DeterminismEvaluator |
There was a problem hiding this comment.
Can we also rename com.facebook.presto.sql.planner.DeterminismEvaluator into ExpressionDeterminismEvaluator and mark it deprecated? That class is a complete hack. It is also incorrect to some extend. It should go away in the future.
| functionManager, | ||
| compiledLambdaMap); | ||
|
|
||
| Variable result = scope.declareVariable(boolean.class, "result"); |
There was a problem hiding this comment.
I might have missed something but RowExpressionCompiler is a quite powerful tool to compile any RowExpression beyond predicates. So, in theory, result could be Object.class. Is it possible to make this class a generic compiler rather than only for predicates?
There was a problem hiding this comment.
@highker It should be possible to change this into a generic expression evaluator, but what's the use case? If a generic version returns an Object, then current use case for the filter would require an undesirable cast to boolean.
mbasmanova
force-pushed
the
predicate-compiler
branch
5 times, most recently
from
38f798b to
aef610c
Compare
|
@highker James, thank you for review. I addressed your comments and updated the PR. Would you take another look? |
| /** | ||
| * Predicate expression may not contain any variable references, only input references. | ||
| */ | ||
| Supplier<Predicate> compilePredicate(RowExpression predicate); |
There was a problem hiding this comment.
As discussed offline, let's put a javadoc to denote a few things:
- this is an experimental API
- the goal is do to filter reordering in connector to workaround lazy block loading everything
- we should explore the possibility having a new interface in lazy block (or maybe new block implementation) to selectively load blocks
There was a problem hiding this comment.
@highker Added javadoc to clarify that this is an experimental API.
/**
* @apiNote An experimental API to allow Hive connector to implement smart
* filtering on the encoded data to avoid materializing data unnecessarily.
*/
public interface PredicateCompiler
{
mbasmanova
force-pushed
the
predicate-compiler
branch
from
aef610c to
d7294bb
Compare
No description provided.