Non equality predicates in outer join (v3)

[losipiuk]

Supersedes #4950

[martint]

Have you benchmarked this? Without compiling the join expressions, it may be a big performance hit for queries that push the filter into the join.

[losipiuk]

No - I did not. I expect that performance will not be great. Though the penalty will only apply to queries which would not execute at all before this change.
Before adding support for join-filter calling to generated classes I wanted to pass this patch through you to validate overall approach.

[losipiuk]

@martint. Friendly ping. Please take a look at interpreted versions so we can proceed with making changes to class generators.

[martint]

@losipiuk, the general approach is fine. I'm a bit concerned about potential performance projects. For instance, in InMemoryJoinHash.getNextJoinPositionFrom (https://github.com/prestodb/presto/pull/5088/files#diff-cc58d9f8668f9f278dae5330c9e4d7a6R137), if that code path is hit from executions that have a filter function and ones that don't, the branch prediction may not work well and affect performance for regular joins.

[losipiuk]

@martint
For this exact case we could possibly have two distinct LookupSource implementations:
InMemoryHashJoin and FilteringInMemoryHashJoin. And select implementation in PagesIndex#createLookupSource(). There is a chance that it would behave better than branch in InMemoryHashJoin.

It would be nice to benchmark that. This seems a bit hard to me though without actually running query set on real cluster.

[losipiuk]

@martint I separated the implementations. (commit 195a56a).

This patch uses inheritance.
Other approach would be to have single implementation of InMemoryJoinHash but inject it the implementation of interface which would be used in place of getNextJoinPositionFrom.
IMO neither is very nice. Let me know if you would rather see composition here.

[losipiuk]

I rebased this onto master.

[martint]

@losipiuk, what parts changed compared to the last version? Are the changes mostly in "Separate filtering/non-filtering LookupSource implementations"?

[martint]

Can you remove the hotspot_pidXXXX.log files from this commit: edfdfa7? Github can't handle them and it refuses to show the diff.

[losipiuk]

@martint. Apart from simple conflict resolutions the changes are in "Separate filtering/non-filtering LookupSource implementations" commit. It addresses your concern of branching within InMemoryJoinHash. This commit could probably be merged with parent. I just left it as separate for easier reviewing.

I removed the hotspot logs. Sorry about that :).

[martint]

Looking at this a bit closer, we may not actually need to split the two implementations. In JoinCompiler, we load the InMemoryJoinHash in an isolated classloader for each query, so profile pollution shouldn't happen.

In any case, if we weren't doing that, this implementation wouldn't help much -- the "loop" is not part of the LookupSource, so the call would get virtualized for multiple implementations of that interface.

Sorry about the churn.

[losipiuk]

@martint thanks for clarification. Please let me know if I am getting this right.

Was IsolatedClass mechanism for InMemoryJoinHash in JoinCompiler originally introduced to avoid polymorphic virtual calls to methods of LookupSource?
Does it play well with fact that later in the code path the LookupSource is wrapped (or not) with OuterLookupSource/PartitionedLookupSource...?

Or is the latter problem solved with some other classloader mangling?

Or the IsolateClass mechanism has nothing to do with virtual calls?

Sorry for possibly dumb questions but I wanted to get better understanding on that.

[losipiuk]

I removed the commit splitting the implementations. Is that good to be merged?

[martint]

Yes, the IsolatedClass mechanism is specifically to avoid virtual calls and profile pollution. It's a simple way to generate a class by copying the bytecodes from an existing class.

Does it play well with fact that later in the code path the LookupSource is wrapped (or not) with OuterLookupSource/PartitionedLookupSource...?

It doesn't matter. The entire join operator is generated in the same way, so the loops should be specialized for each query.

[losipiuk]

Cool - thanks :)

[losipiuk]

Btw @martint. We discovered a bug in presto (I think introduced by me - but I did not check git log ;) ).

Queries like this one:

 select * from test1 a left join test2 b on a.col1=b.col1 and b.col2=b.col2

should be failing.
But are executed currently - producing wrong results.
This patch fixes that.

[martint]

Merged, thanks!

[buremba]

Is there any plan to improve support for non-equity predicates in outer join? I tested it and the performance is actually worse than equity joins.

I simply put a false expression in JOIN clause and it took x2 time to execute the query.

> select count(*) from tweet left join (select 1 as id) mapping on (false and tweet.id = mapping.id);

Query 20160802_133540_00030_kwbyd, FINISHED, 1 node
Splits: 131 total, 131 done (100.00%)
0:29 [39.9M rows, 173MB] [1.36M rows/s, 5.88MB/s]

> select count(*) from tweet left join (select 1 as id) mapping on (tweet.id = mapping.id);

Query 20160802_133406_00027_kwbyd, FINISHED, 1 node
Splits: 131 total, 131 done (100.00%)
0:15 [39.9M rows, 316MB] [2.61M rows/s, 20.7MB/s]

Also, even though the right side of the JOIN only has one row, JOIN has a big performance impact on the query:

> select count(*) from tweet where id is not null;

Query 20160802_134119_00035_kwbyd, FINISHED, 1 node
Splits: 128 total, 128 done (100.00%)
0:02 [39.9M rows, 316MB] [20M rows/s, 159MB/s]

[kbajda]

Yes, our next step is to optimize performance of such predicates. Will create issues/PRs soon.
CC: @sopel39 @losipiuk

[buremba]

Thanks, looking forward to it.

[losipiuk]

@buremba: here is the PR #5810. It fixes the performance for case where there is at least one equality based conjunct in join codition.

[buremba]

@losipiuk thanks, I'll look into it. Hope that it can be merged soon.