Extend fast inequality join #630
Conversation
|
@anu can you post what performance results you had before applying your patch? |
| @@ -121,6 +121,13 @@ public void setUp() | |||
| .execute("SELECT count(*) FROM t1 JOIN t2 on (t1.bucket = t2.bucket) AND t1.val1 < sin(t2.val2)"); | |||
| } | |||
|
|
|||
| @Benchmark | |||
There was a problem hiding this comment.
should not it belong to previous commit?
| @@ -90,9 +91,13 @@ public JoinHash get() | |||
| // are not thread safe... | |||
| Optional<JoinFilterFunction> filterFunction = | |||
| filterFunctionFactory.map(factory -> factory.create(session.toConnectorSession(), addresses, channels)); | |||
| List<JoinFilterFunction> filterExpressions = filterFunctionFactory.isPresent() ? | |||
There was a problem hiding this comment.
Use Optional.map as in the statement above instead of ternary operator
| @@ -81,6 +82,61 @@ public void testGetSortExpression() | |||
| ComparisonExpressionType.GREATER_THAN, | |||
| new FunctionCall(QualifiedName.of("sin"), ImmutableList.of(new SymbolReference("b1"))), | |||
| new SymbolReference("p1"))); | |||
|
|
|||
| assertGetSortExpression( | |||
| new LogicalBinaryExpression(LogicalBinaryExpression.Type.OR, | |||
There was a problem hiding this comment.
instead of creating expressions by hand you can use something like:
expression("b1 > p1 OR b2 <= p1")
where expression is:
private Expression expression(String sql) {
return rewriteIdentifiersToSymbolReferences(new SqlParser().createExpression(sql));
}
|
Why is that targeted for |
| @@ -107,6 +107,20 @@ public void setUp() | |||
| .execute("SELECT count(*) FROM t1 JOIN t2 on (t1.bucket = t2.bucket) WHERE t1.val1 < t2.val2"); | |||
| } | |||
|
|
|||
| @Benchmark | |||
| public List<Page> benchmarkJoinWithExpressionPredicate(Context context) | |||
There was a problem hiding this comment.
I would explicitly state we are talking about arithmetics here. Maybe call method benchmarkJoinWithArithmeticInPredicate
There was a problem hiding this comment.
It seems you renamed the wrong test method. The one with sin(). But left original name for test method with arithmetic.
| @@ -2279,6 +2279,8 @@ public void testJoinWithLessThanInJoinClause() | |||
| "VALUES -1"); | |||
| // test with only null value in build side | |||
| assertQuery("SELECT b FROM nation n, (VALUES (0, NULL)) t(a, b) WHERE n.regionkey - 100 < t.b AND n.nationkey = t.a", "SELECT 1 WHERE FALSE"); | |||
| // test with filter expression | |||
There was a problem hiding this comment.
Shouldn't comment here be sth like:
// test with function calls in predicate
| @@ -2298,6 +2300,8 @@ public void testJoinWithGreaterThanInJoinClause() | |||
| "VALUES -1"); | |||
| // test with only null value in build side | |||
| assertQuery("SELECT b FROM nation n, (VALUES (0, NULL)) t(a, b) WHERE n.regionkey + 100 > t.b AND n.nationkey = t.a", "SELECT 1 WHERE FALSE"); | |||
| // test with filter expression | |||
| return new SortExpressionVisitor(buildSymbols).process(filter); | ||
| } | ||
|
|
||
| private static class SortExpressionVisitor |
There was a problem hiding this comment.
I pushed the fixup commit 518d7ea to PR branch which simplifies the extractor. See what you think.
I found the visitExpression logic with HashSet hard to follow. You can naturally put logic of supporting just AND with matching sort expressions within visitLogicalBinaryExpression.
Take a look at commit and see what you think.
| return new FilterExpressions().process(expression); | ||
| } | ||
|
|
||
| private static class FilterExpressions |
| @Override | ||
| protected List<Expression> visitExpression(Expression expression, Void context) | ||
| { | ||
| List<Expression> filters = new ArrayList<>(); |
There was a problem hiding this comment.
You can do it in more functional way (IMO more a bit more readable) as:
return expression.getChildren().stream()
.flatMap(child -> process(child).stream())
.collect(toImmutableList());| @@ -152,13 +153,14 @@ public Factory build() | |||
| return new Factory() | |||
| { | |||
| @Override | |||
| public PositionLinks create(Optional<JoinFilterFunction> lessThanFunction) | |||
| public PositionLinks create(Optional<JoinFilterFunction> lessThanFunction, List<JoinFilterFunction> filterFunctions) | |||
There was a problem hiding this comment.
I do not understand why we keep old lessThanFunction and add a list of filterFunctions.
Would lessThanFunction not be on of the functions in filterFunctions list?
There was a problem hiding this comment.
We use the lessThanFunction (a<b AND b< a+10 in case of range predicates), for the next() method. I can get rid of it and use filterFunctions ({a< b, b< a+10}) in a loop in next() method too (like I do in start()).
There was a problem hiding this comment.
I would rename filterFunctions to inequalityFilterConjuncts or inequalityFilterExpressions and then either remove lessThanFunction.
Or rename it to combinedIneqalityFIlterExpressions and ensure that it is clear when this is constructed that it is just a conjunction of function passed in the other parameter.
|
This is not going to sprint-59. I had the PR branch locally rebased on that. I will change it once the review is done |
| @Override | ||
| protected List<Expression> visitLogicalBinaryExpression(LogicalBinaryExpression expression, Void context) | ||
| { | ||
| return extractConjuncts(expression); |
There was a problem hiding this comment.
This does not seem good.
It will work correctly for case like this
a < x AND a < x +10
But what if we have other conjuncts in filter function. Which are not in shape of
[sort_symbol] [<=|>=|<|>] [expression using probe side symbols>].
Then those conjuncts will be used as filter functions in SortedPositionLInks.start(). And they will not work fine when passed as lessThanFunction to binary search.
Am I right?
For sure we need tests for that.
There was a problem hiding this comment.
as discussed added test in ATQ.
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
3 times, most recently
from
cbc96f1
to
ce681df
Compare
|
Can you please squash fixup commits? |
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
from
ce681df
to
da032be
Compare
|
@kokosing done |
| @@ -107,6 +107,20 @@ public void setUp() | |||
| .execute("SELECT count(*) FROM t1 JOIN t2 on (t1.bucket = t2.bucket) WHERE t1.val1 < t2.val2"); | |||
| } | |||
|
|
|||
| @Benchmark | |||
| public List<Page> benchmarkJoinWithExpressionPredicate(Context context) | |||
There was a problem hiding this comment.
It seems you renamed the wrong test method. The one with sin(). But left original name for test method with arithmetic.
| @@ -2279,6 +2279,8 @@ public void testJoinWithLessThanInJoinClause() | |||
| "VALUES -1"); | |||
| // test with only null value in build side | |||
| assertQuery("SELECT b FROM nation n, (VALUES (0, NULL)) t(a, b) WHERE n.regionkey - 100 < t.b AND n.nationkey = t.a", "SELECT 1 WHERE FALSE"); | |||
| // test with function calls in predicate | |||
There was a problem hiding this comment.
Please explain in the comment what are you testing here. This is not supported case for inequality fast joins.
There was a problem hiding this comment.
This case is supported by this optimization since the non-equi join condition occurs in the ON clause. updated the comment.
| @@ -2305,6 +2305,22 @@ public void testJoinWithGreaterThanInJoinClause() | |||
| } | |||
|
|
|||
| @Test | |||
| public void testJoinWithRangePredicatesinJoinClause() | |||
| @@ -2305,6 +2305,22 @@ public void testJoinWithGreaterThanInJoinClause() | |||
| } | |||
|
|
|||
| @Test | |||
| public void testJoinWithRangePredicatesinJoinClause() | |||
| { | |||
| assertQuery("SELECT COUNT(*)\n" + | |||
There was a problem hiding this comment.
Please format the queries so every condition is in separate line. This is hard to read in this shape.
Also add comment what is this test testing.
I understand that it for testing regression in fast inequality join code.
And the code is working for this query because expressions used in equality conditions are rewritten to symbols. So actual expression in join is in shape of build_symbol < probe_expression.
What about seconde query?
| @@ -43,7 +42,8 @@ | |||
| * | |||
| * filterFunction_1(...) OR filterFunction_2(....) OR ... OR filterFunction_n(...) | |||
| * | |||
| * To use lessThanFunction in this class, it must be an expression in form of: | |||
| * To use the elements in the list of filters inequalityJoinFilterConjuncts in this class, | |||
There was a problem hiding this comment.
Actually this Javadoc is out of sync with the codebase. As we are not supporting g(build_column, ....) but only build_column as sort expression. Maybe add a commit which puts that javadoc in sync with current implementation?
There was a problem hiding this comment.
We do support g(build_column) as sort expression, provided the expression is pushed to the Scan node (eg: when the expression appears in the ON clause). Updated the doc accordingly.
There was a problem hiding this comment.
Yeah - but in this case it is essentially a symbol when it comes to filter function in join.
I would rather keep in javadoc that g(b_symbol) is not supported. As this javadoc is describing what is supported locally.
It should not need to be updated anytime we add some optimizer in other place which transforms expression which does not have supported shape to one which has.
Then this Javadoc would be unmaintainble (if it is not such already :) )
| return startingPosition; | ||
| } | ||
|
|
||
| private int findStartPositionForFunction(int startingPosition, int probePosition, Page allProbeChannelsPage, JoinFilterFunction filterFunction) |
There was a problem hiding this comment.
Make paremeter ordering consistent with applyLessThanFunction. I.e. make filterFunction first parameter.
| } | ||
|
|
||
| @Override | ||
| public int start(int startingPosition, int probePosition, Page allProbeChannelsPage) | ||
| { | ||
| int count = inequalityJoinFilterConjuncts.size(); | ||
| while (count > 0) { |
| @@ -112,7 +114,14 @@ public JoinFilterFunctionFactory compileJoinFilterFunction(RowExpression filter, | |||
| private JoinFilterFunctionFactory internalCompileFilterFunctionFactory(RowExpression filterExpression, int leftBlocksSize, Optional<SortExpression> sortChannel) | |||
| { | |||
| Class<? extends InternalJoinFilterFunction> internalJoinFilterFunction = compileInternalJoinFilterFunction(filterExpression, leftBlocksSize); | |||
| return new IsolatedJoinFilterFunctionFactory(internalJoinFilterFunction, sortChannel); | |||
|
|
|||
| List<Class<? extends InternalJoinFilterFunction>> internalInequalityJoinFilterConjuncts = new ArrayList<>(); | |||
There was a problem hiding this comment.
What about using Optional.map()
List<Class<? extends InternalJoinFilterFunction>> internalInequalityJoinFilterConjuncts =
sortChannel.map(channel -> channel.getInequalityJoinFilterConjuncts().stream()
.map(rowExpression -> compileInternalJoinFilterFunction(rowExpression, leftBlocksSize))
.collect(toImmutableList()))
.orElse(ImmutableMap.of());If you do not like this please at least use ImmutableMap.of() instead new ArrayList(). We use ImmutableMap.of() as empty list by convention.
There was a problem hiding this comment.
tried this, but had to change the type of internalInequalityJoinFilterConjuncts to ImmutableList which also needs changing the constructor of IsolatedJoinFilterFunctionFactory. So keeping it as-is.
| @@ -71,6 +72,7 @@ | |||
| import static com.facebook.presto.sql.gen.TryCodeGenerator.defineTryMethod; | |||
| import static com.google.common.base.MoreObjects.toStringHelper; | |||
| import static com.google.common.base.Verify.verify; | |||
| import static com.google.common.collect.ImmutableList.toImmutableList; | |||
There was a problem hiding this comment.
JoinFilterCacheKey currently has sortChannel field?
It seems unnecessary to me. Anyway, it should either be removed. Or taken into consideration in equals and hashCode.
It is not related strictly to this PR but maybe you could fix that that along the way as you are working on this class anyway. As a separate commit of course :).
There was a problem hiding this comment.
The sortChannel is still being used to call internalCompileFilterFunctionFactory. So added it to equals and hashcode
| import static com.google.common.collect.ImmutableSet.toImmutableSet; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * Currently this class handles only simple expressions like: | ||
| * | ||
| * <p> |
There was a problem hiding this comment.
Not up to date. Probe side expression can be aribtrary one. Not just symbol.
There was a problem hiding this comment.
Added a new javadoc commit to reflect what was supported, and later added info about range predicate support.
|
And one more question. How did evaluating conjuncts one by one influence performance? |
|
no regression after evaluating conjuncts one by one . Here is the result: I will address the rest of the comments. |
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
2 times, most recently
from
f7e97a4
to
1c62815
Compare
| @@ -43,7 +42,8 @@ | |||
| * | |||
| * filterFunction_1(...) OR filterFunction_2(....) OR ... OR filterFunction_n(...) | |||
| * | |||
| * To use lessThanFunction in this class, it must be an expression in form of: | |||
| * To use the elements in the list of filters inequalityJoinFilterConjuncts in this class, | |||
There was a problem hiding this comment.
We do support g(build_column) as sort expression, provided the expression is pushed to the Scan node (eg: when the expression appears in the ON clause). Updated the doc accordingly.
| @@ -107,6 +107,20 @@ public void setUp() | |||
| .execute("SELECT count(*) FROM t1 JOIN t2 on (t1.bucket = t2.bucket) WHERE t1.val1 < t2.val2"); | |||
| } | |||
|
|
|||
| @Benchmark | |||
| public List<Page> benchmarkJoinWithExpressionPredicate(Context context) | |||
| import static com.google.common.collect.ImmutableSet.toImmutableSet; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * Currently this class handles only simple expressions like: | ||
| * | ||
| * <p> |
There was a problem hiding this comment.
Added a new javadoc commit to reflect what was supported, and later added info about range predicate support.
| @@ -71,6 +72,7 @@ | |||
| import static com.facebook.presto.sql.gen.TryCodeGenerator.defineTryMethod; | |||
| import static com.google.common.base.MoreObjects.toStringHelper; | |||
| import static com.google.common.base.Verify.verify; | |||
| import static com.google.common.collect.ImmutableList.toImmutableList; | |||
There was a problem hiding this comment.
The sortChannel is still being used to call internalCompileFilterFunctionFactory. So added it to equals and hashcode
| @@ -2279,6 +2279,8 @@ public void testJoinWithLessThanInJoinClause() | |||
| "VALUES -1"); | |||
| // test with only null value in build side | |||
| assertQuery("SELECT b FROM nation n, (VALUES (0, NULL)) t(a, b) WHERE n.regionkey - 100 < t.b AND n.nationkey = t.a", "SELECT 1 WHERE FALSE"); | |||
| // test with function calls in predicate | |||
There was a problem hiding this comment.
This case is supported by this optimization since the non-equi join condition occurs in the ON clause. updated the comment.
| @@ -112,7 +114,14 @@ public JoinFilterFunctionFactory compileJoinFilterFunction(RowExpression filter, | |||
| private JoinFilterFunctionFactory internalCompileFilterFunctionFactory(RowExpression filterExpression, int leftBlocksSize, Optional<SortExpression> sortChannel) | |||
| { | |||
| Class<? extends InternalJoinFilterFunction> internalJoinFilterFunction = compileInternalJoinFilterFunction(filterExpression, leftBlocksSize); | |||
| return new IsolatedJoinFilterFunctionFactory(internalJoinFilterFunction, sortChannel); | |||
|
|
|||
| List<Class<? extends InternalJoinFilterFunction>> internalInequalityJoinFilterConjuncts = new ArrayList<>(); | |||
There was a problem hiding this comment.
tried this, but had to change the type of internalInequalityJoinFilterConjuncts to ImmutableList which also needs changing the constructor of IsolatedJoinFilterFunctionFactory. So keeping it as-is.
| @@ -211,17 +210,35 @@ public int next(int position, int probePosition, Page allProbeChannelsPage) | |||
| return -1; | |||
| } | |||
| // break a position links chain if next position should be filtered out | |||
| if (applyLessThanFunction(nextPosition, probePosition, allProbeChannelsPage)) { | |||
| return nextPosition; | |||
| int count = inequalityJoinFilterConjuncts.size(); | |||
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
from
1c62815
to
66ae46c
Compare
| .map(rowExpression -> compileInternalJoinFilterFunction(rowExpression, leftBlocksSize)) | ||
| .collect(toImmutableList()); | ||
| } | ||
| List<? extends Class<? extends InternalJoinFilterFunction>> internalInequalityJoinFilterConjuncts = sortChannel.map(channel -> channel.getInequalityJoinFilterConjuncts().stream() |
|
Travis is red. Please change base of the PR to prestodb/master. I think you can do it without creating new one. |
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
from
66ae46c
to
da718de
Compare
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
from
da718de
to
1f0d3bf
Compare
|
travis is green. rebased on master and squashed the commits. |
|
Thanks, I will do one last pass tomorrow and merge if everything looks good. |
| @@ -151,13 +152,10 @@ public Factory build() | |||
| } | |||
| } | |||
|
|
|||
| return lessThanFunction -> { | |||
| checkState(lessThanFunction.isPresent(), "Using SortedPositionLinks without lessThanFunction"); | |||
There was a problem hiding this comment.
please reintroduce the check in new code checking if provided List is not empty.
There was a problem hiding this comment.
Or actually it can be moved to SortedPositionLinks constructor.
There was a problem hiding this comment.
done. closing this and opening a PR upstream
The sorted position links is searched for each of the expression in the range predicate. Thus this optimization works only for predicates with AND (conjuncts). The iteration over the position links is stopped as soon as each of the filter expression is false.
anusudarsan
force-pushed
the
extend-fast-inequality-join
branch
from
1f0d3bf
to
7bf10af
Compare
|
prestodb#8614 created. closing this. |
Follow up PR to extend functionality added in prestodb#7097. The tests result below. @losipiuk Please review.
PR branch
sprint-59 branch
The PR extends the functionality to speed up query with range predicates eg:
benchmarkRangePredicateJoin. But I added benchmark tests for other queries which were already addressed by the optimization. So you can see the comparison below with and without this optimization.