Sort position links for faster non-equi joins #7097

pnowojski · 2017-01-16T08:35:39Z

This is a implementation of:

From preliminary benchmarks this change didn't introduce regression in equality joins and for some in-equality joins this gives orders of magnitude speed ups.

BenchmarkInequalityJoin speeds up from ~210ms down to ~50ms and existing BenchmarkHashBuildAndJoinOperators do not show a visible performance regression:

AFTER

Benchmark                                              (hashColumns)  (matchRate)  Mode  Cnt    Score    Error  Units
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash        varchar          N/A  avgt   10   56.792 ±  7.107  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash         bigint          N/A  avgt   10   35.530 ±  2.304  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash            all          N/A  avgt   10   69.249 ±  7.824  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar          0.1  avgt   10  100.534 ±  8.280  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar            1  avgt   10  127.782 ±  5.132  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar            2  avgt   10  118.533 ± 13.199  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint          0.1  avgt   10   79.058 ±  4.773  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint            1  avgt   10  100.670 ± 10.200  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint            2  avgt   10   74.568 ±  2.282  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all          0.1  avgt   10  119.906 ±  1.313  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all            1  avgt   10  156.546 ± 12.703  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all            2  avgt   10  138.223 ±  7.201  ms/op

BEFORE:

Benchmark                                              (hashColumns)  (matchRate)  Mode  Cnt    Score    Error  Units
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash        varchar          N/A  avgt   10   54.626 ±  5.888  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash         bigint          N/A  avgt   10   35.253 ±  1.619  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkBuildHash            all          N/A  avgt   10   73.002 ±  8.299  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar          0.1  avgt   10   93.097 ±  4.915  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar            1  avgt   10  130.529 ±  7.421  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash         varchar            2  avgt   10  114.086 ± 10.210  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint          0.1  avgt   10   71.442 ±  4.633  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint            1  avgt   10   92.942 ± 12.446  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash          bigint            2  avgt   10   77.394 ±  2.632  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all          0.1  avgt   10  111.427 ±  3.545  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all            1  avgt   10  150.452 ± 15.728  ms/op
BenchmarkHashBuildAndJoinOperators.benchmarkJoinHash             all            2  avgt   10  130.621 ±  3.306  ms/op

There are couple of TODOs (future work?):

compare method may not fit to PagesHashStrategy
improve decision between choosing SortedPositionLinks over PositionLinksList
maybe add NoopPositionLinks (that returns always -1) if all rows in build table have unique key (this would reduce memory usage and it might slightly improve performance)
add similar logic to cross join

losipiuk · 2017-01-16T12:44:02Z

presto-main/src/main/java/com/facebook/presto/operator/PositionLinksList.java

+
+import static io.airlift.slice.SizeOf.sizeOf;
+
+public final class PositionLinksList


Why do you need this. It incomplete and unused.

Why is it incomplete? It is used both in SortedPositionLinks and in LookupSource directly, in case if there is no SortChannel for filter function.

losipiuk · 2017-01-16T12:44:13Z

presto-main/src/main/java/com/facebook/presto/operator/PositionLinks.java

+
+import java.util.Optional;
+
+public interface PositionLinks


Add short javadoc what it is.

please check comment if it suffice

losipiuk · 2017-01-16T12:46:01Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+    @Override
+    public void setFilterFunction(Optional<JoinFilterFunction> filterFunction)
+    {
+        checkState(filterFunction.isPresent());


Either change signature to take "JoinFilterFunction" or allow empty() and set null to the field in that case.

changed method signature

losipiuk · 2017-01-16T12:49:34Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortChannelExtractor.java

+                    break;
+                case GREATER_THAN:
+                case GREATER_THAN_OR_EQUAL:
+                    descending = true;


don't do that. Extract separate isDescending(ComparisonType) method.

losipiuk · 2017-01-16T12:50:10Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortChannelExtractor.java

+            ComparisonExpression comparison = (ComparisonExpression) filter;
+            boolean descending = false;
+            switch (comparison.getType()) {
+                case EQUAL:


drop those and just add default: section with return Optional.empty()

losipiuk · 2017-01-16T12:51:25Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortChannelExtractor.java

+        return Optional.empty();
+    }
+
+    private static Optional<Integer> isBuildFieldReference(Set<Integer> buildLayout, Expression expression)


rename to asBuildFieldReference

losipiuk · 2017-01-16T12:55:09Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortChannelExtractor.java

+                    if (sortChannel.isPresent()) {
+                        return Optional.of(new SortChannel(sortChannel.get(), descending));
+                    }
+                    return Optional.empty();


Shouldn't you check some conditions about other side which is not buildFieldReference.
It seems like you are making some non-written assumption on shape of expression which is passed to this method.
Those should be exposed as Preconditions.checkArgument calls.

yes, I have missed one thing. For such simple extractor, I have to check that there is no build field references on the other side of the inequality. Fixed

losipiuk · 2017-01-16T12:59:49Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

@@ -1511,7 +1511,7 @@ private LookupSourceFactory createLookupSourceFactory(
            List<Integer> buildChannels = ImmutableList.copyOf(getChannelsForSymbols(buildSymbols, buildSource.getLayout()));
            Optional<Integer> buildHashChannel = buildHashSymbol.map(channelGetter(buildSource));

-            Optional<JoinFilterFunctionFactory> filterFunctionFactory = node.getFilter()
+            Optional<JoinFilterFunctionCompiler.JoinFilterFunctionFactory> filterFunctionFactory = node.getFilter()


why this change?

+1 for undo

unintended change. Reverted

losipiuk · 2017-01-16T13:04:07Z

presto-main/src/main/java/com/facebook/presto/operator/SimplePagesHashStrategy.java

+    @Override
+    public int compare(int leftBlockIndex, int leftBlockPosition, int rightBlockIndex, int rightBlockPosition)
+    {
+        throw new NotImplementedException();


Please add as a reference implementation.

This also should be marked as //TODO

you were able to write compiler code, so it should be easy to write regular code, shouldn't it?

At a time it wasn't crucial for performance tests and validation whether whole idea has any sense. I have added it now though.

losipiuk · 2017-01-16T13:31:26Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHash.java

@@ -71,6 +76,7 @@ public long getJoinPosition(int position, Page hashChannelsPage, Page allChannel
    @Override
    public long getJoinPosition(int position, Page hashChannelsPage, Page allChannelsPage, long rawHash)
    {
+        first = true;


This cannot be part of state as class is shared between threads.

Yes, that's one of the reasons why I think decoupling PositionLinks from JoinHash is a way to go. I should have marked this as a //TODO

kokosing

initial review

kokosing · 2017-01-17T06:06:11Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+{
+    private final PositionLinks positionLinks;
+    private final int[][] compactedPositionLinks;
+    private final Map<Integer, List<Integer>> positionLinksBuilder;


You can use multimap. Does it influence that you are using boxed types?

I can not use Multimap, because I need to have last linked element as a starting point for the position links. Thus on each linking I would have to copy all collection returned by Multimap.get(...)

kokosing · 2017-01-17T06:13:50Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+    @Override
+    public void link(int from, int to)
+    {
+        List<Integer> links = positionLinksBuilder.containsKey(to) ? positionLinksBuilder.get(to) : new ArrayList<>();


You can use computeIfAbsent - https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-

kokosing · 2017-01-17T06:26:37Z

presto-tests/src/test/java/com/facebook/presto/tests/TestLocalQueries.java

+    {
+        assertQuery(
+                "select o.orderkey, o.orderdate, l.shipdate from orders o, lineitem l where l.orderkey=o.orderkey and l.shipdate < o.orderdate + INTERVAL '10' DAY",
+                "select o.orderkey, o.orderdate, l.shipdate from orders o, lineitem l where l.orderkey=o.orderkey and l.shipdate < DATEADD('DAY', 10, o.orderdate)");


Generally, I think these tests are not enough. It is similar with the current problem related to Exchanges.
The query will pass regardless you other commits are merged or not.

I am sure there are already some tests form inequality join.

Tests are very WIP.

Btw, there doesn't seems to be tests, that cover those test cases with simple enough comparison expression :)

kokosing · 2017-01-17T06:28:13Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortChannelExtractor.java

+    {
+    }
+
+    public static Optional<SortChannel> getSortChannel(Map<Symbol, Integer> buildLayout, Expression filter)


I think you can easily add unit tests for that class. That would be nice because it would show expressions are supported.

kokosing · 2017-01-17T06:31:55Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

@@ -1511,7 +1511,7 @@ private LookupSourceFactory createLookupSourceFactory(
            List<Integer> buildChannels = ImmutableList.copyOf(getChannelsForSymbols(buildSymbols, buildSource.getLayout()));
            Optional<Integer> buildHashChannel = buildHashSymbol.map(channelGetter(buildSource));

-            Optional<JoinFilterFunctionFactory> filterFunctionFactory = node.getFilter()
+            Optional<JoinFilterFunctionCompiler.JoinFilterFunctionFactory> filterFunctionFactory = node.getFilter()


+1 for undo

kokosing · 2017-01-17T06:33:04Z

presto-main/src/test/java/com/facebook/presto/operator/TestHashJoinOperator.java

+                    @Override
+                    public Optional<SortChannelExtractor.SortChannel> getSortChannel()
+                    {
+                        return Optional.empty();


why empty? I think it would be nice to test the things here.

I didn't want to spend a lot of time adding tests without initial approval that this thing will go into code base.

Is that still valid? I mean there is approval on approach. Right?

I think is not worth testing, since writing such test would be quite painful.

There are unit tests for both SortedPositionLinks and SortChannelExtractor and integration between JoinHash and PositionLinks interface is tested here regardless of this Optional.empty(). Also there are integration tests in AbstractTestQueries for this .

kokosing · 2017-01-17T06:35:07Z

presto-main/src/main/java/com/facebook/presto/operator/SimplePagesHashStrategy.java

+    @Override
+    public int compare(int leftBlockIndex, int leftBlockPosition, int rightBlockIndex, int rightBlockPosition)
+    {
+        throw new NotImplementedException();


you were able to write compiler code, so it should be easy to write regular code, shouldn't it?

kokosing · 2017-01-17T06:52:57Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHashSupplier.java

+        if (filterFunctionFactory.isPresent() && filterFunctionFactory.get().getSortChannel().isPresent()) {
+            this.positionLinks = new SortedPositionLinks(
+                    addresses.size(),
+                    (leftPosition, rightPosition) -> {


extract this as separate class

pnowojski · 2017-03-27T11:54:50Z

@dain I have fixed concurrency issues without refactor (as discussed) and it's read for review.

dain · 2017-03-27T17:19:49Z

@losipiuk and @kokosing are you two done with your reviews?

losipiuk · 2017-03-27T21:39:48Z

@dain I will give it one more read but feel free to go through it at the same time.

losipiuk

Looks ok up to my understanding of the code.
I have some outstanding questions to @pnowojski to be asked offline.
Though, It should not block your review @dain - at least to extent to see if general approach is something we want to push forward or not.

losipiuk · 2017-03-28T07:46:04Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/MemoryLocalQueryRunner.java

@@ -47,10 +46,10 @@ public void execute(@Language("SQL") String query)
                .setSystemProperty("optimizer.optimize-hash-generation", "true")
                .build();
        ExecutorService executor = localQueryRunner.getExecutor();
-        MemoryPool memoryPool = new MemoryPool(new MemoryPoolId("test"), new DataSize(1, GIGABYTE));
-        MemoryPool systemMemoryPool = new MemoryPool(new MemoryPoolId("testSystem"), new DataSize(1, GIGABYTE));
+        MemoryPool memoryPool = new MemoryPool(new MemoryPoolId("test"), new DataSize(2, GIGABYTE));


Maybe extract 1 variable (e.g MAX_MEMORY_USED). And set the pools as fractions of this one. So if we need to change sth here we just change one thing. Also set max-data-per-node config prop based on that.

I would rather expect that in the future we will need more customization of those parameters. In other words, we will need passing those values as parameters. Binding them to one static value doesn't seem like a good idea, since they are quite independent.

losipiuk · 2017-03-28T07:47:39Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/BenchmarkInequalityJoin.java

+
+        public void run()
+        {
+            queryRunner.execute("SELECT count(*) FROM t1 a, t2 b WHERE a.bucket = b.bucket AND a.val1 < b.val2");


don't we need to save the results of query into some field to be sure that JVM does not need some weird code-removal optimizations?

I have added collecting output into List<Page> to MemoryQueryRunner

losipiuk · 2017-03-28T07:50:19Z

presto-main/src/main/java/com/facebook/presto/operator/PositionLinks.java

+         * true for chosen right value. User should later use this reference value for
+         * starting the iteration over position links.
+         */
+        boolean link(int left, int right);


Why not return just new head instead boolean (either left or right)

losipiuk · 2017-03-28T07:58:24Z

presto-main/src/main/java/com/facebook/presto/operator/PositionLinks.java

+
+    /**
+     * Iterate over position links.
+     */


How end of list is signalled?

losipiuk · 2017-03-28T08:29:16Z

presto-main/src/main/java/com/facebook/presto/operator/PositionLinksList.java

+import static io.airlift.slice.SizeOf.sizeOf;
+import static java.util.Objects.requireNonNull;
+
+public final class PositionLinksList


Commit message suggests that you use Javas LinkedList as an implementation.
Maybe use Implement PositionLinks as link array as commit message and name the class PostionLinksArray?

Or maybe LinkedPositionLinks will be good and match the SortedPositionLinks from latter commit?

I have renamed it to ArrayPositionLinks, since it matches with Java's convention of ArrayList vs List.

losipiuk · 2017-03-28T09:19:59Z

presto-main/src/test/java/com/facebook/presto/operator/TestPositionLinks.java

+    public void testSortedPositionLinks()
+    {
+        SortedPositionLinks.Builder builder = SortedPositionLinks.builder(
+                1000,


InelliJ says Can be replaced with Comparator.comparingLong

yes, but I don't like it. This seems more easier to understand for me and it matches to reversed comparator in test below.

losipiuk · 2017-03-28T09:48:59Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -2004,6 +2004,31 @@ public void testHistogram()
    }

    @Test
+    public void testInequalityJoin()


Name this testJoinWithLessThanInJoinClause

Inequality is everything which is not equailty. E.g. LIKE, complex expressions etc.
At least it used that way in test name convention so far.

losipiuk · 2017-03-28T09:49:22Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

+    }
+
+    @Test
+    public void testReversedInequalityJoin()


losipiuk · 2017-03-28T09:49:26Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

+    }
+
+    @Test
+    public void testInequalityJoinWithDate()


losipiuk · 2017-03-28T09:55:59Z

presto-main/src/test/java/com/facebook/presto/operator/TestHashJoinOperator.java

+                    @Override
+                    public Optional<SortChannelExtractor.SortChannel> getSortChannel()
+                    {
+                        return Optional.empty();


Is that still valid? I mean there is approval on approach. Right?

dain · 2017-03-31T17:48:31Z

dain

Nice. I had a bunch of minor comments, but it is generally looking good. I noted one major concern about the stateful filter functions, but I think this will be easy to remedy.

This PR was pretty difficult to read because the features were broken into many small commits containing a single file. The is especially the case when a new class is added in one commit and then the test is in the next. It helps to read the code quickly if the commits grouped into features, so I can see how the different bits relate to each other and see usages in code and tests. For this review, I had to flip back and forth between commits. The collection into features (maybe mini-features) helps be focus on a single context which makes reading faster. Here is how I would have grouped the commits, and how I think you should squash them before we merge them:

Fix benchmarks using MemoryLocalQueryRunner

3533940 Collect output of the memory benchmarks
Also apply the "output" fix to the other benchmarks using MemoryLocalQueryRunner

Increase memory limits for benchmarks using MemoryLocalQueryRunner

a87c482 Bump memory limits for memory benchmarks

Add inequality join benchmarks

5fd096a Add BenchmarkInequalityJoin
edc1c75 Add benchmark for using position links in join

Add direct handling of inequality conditions in JoinHash

6b538a7 Add PositionLinks interface
ca87b79 Implement ArrayPositionLinks
b2e85b6 Implemented SortedPositionLinks with binary search
3d33e0b Add unit tests for PositionLinks
2ee7111 Add SortExpressionExtractor
3f9d0df Add unit tests for SortExpressionExtractor
87b119a Add tests queries for sorting position links
29b2f67 Extract sort channel from filter function
f20ac3a Add compare method to PagesHashStrategy
4486d5e Use SortedPositionLinks in LookupJoinOperator

dain · 2017-03-31T19:45:06Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/MemoryLocalQueryRunner.java

+        ImmutableList.Builder<Page> output = ImmutableList.builder();
+        List<Driver> drivers = localQueryRunner.createDrivers(
+                query,
+                (operatorId, planNodeId, types, pagePreprocessor, serdeFactory) -> new PagesCollectorFactory(operatorId, planNodeId, types, output),


Maybe add an implementation of OutputFactory an inner class, so you don't have to have this crazy 5 argument lambda :) ?

dain · 2017-03-31T19:46:42Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/MemoryLocalQueryRunner.java

@@ -89,4 +103,106 @@ private static LocalQueryRunner createMemoryLocalQueryRunner()

        return localQueryRunner;
    }
+
+    private static class PagesCollectorFactory


I think you might be able to use PageConsumerOutputFactory instead of this.

I still "you might be able to use PageConsumerOutputFactory instead of this".

dain · 2017-03-31T19:54:53Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHashSupplier.java

@@ -67,7 +84,33 @@ public double getExpectedHashCollisions()
    @Override
    public JoinHash get()
    {
-        Optional<JoinFilterFunction> filterFunction = filterFunctionFactory.map(factory -> factory.create(session, addresses, channels));


I'm am concerned about this change. The reason we have a FilterFunctionFactory is because filter functions are stateful and not thread safe. Each driver (and thus threads) calls get to fetch a private version of the JoinHash for their thread. I am surprised there are no join tests that use a stateful function in the join condition (or maybe the tests are not concurrent enough to see the problem).

Grrr... :( This makes it more complicated. I can not build positionLinks in this get method, because it would be too costly to sort position links every time get is called.

But on the other hand, I can not return positionLinks created in the constructor with it's private final filterFunction here, because then we hit the same problem you are describing.

The only solution that I can see is to build/sort positionLinks in the constructor with temporarily created filterFunction, but before returning it in in get, this filter function must be overwritten by newly created one.

dain · 2017-03-31T19:56:37Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHashSupplier.java

+    }
+
+    public static class PositionComparator
+            implements Comparator<Integer>


Why not use IntComparator from fastutils?

dain · 2017-03-31T21:17:44Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/BenchmarkInequalityJoin.java

+
+        public List<Page> run()
+        {
+            return queryRunner.execute("SELECT count(*) FROM t1 a, t2 b WHERE a.bucket = b.bucket AND a.val1 < b.val2");


I think we normally put this call in the @Benchmark method and have the context just expose a getter for the runner.

It might be easier to read if you named the tables a and b or if you dropped the aliases and rewrite the query to:

SELECT count(*) FROM t1 JOIN t2 using (bucket) WHERE t1.val1 < t2.val2

dain · 2017-04-01T01:29:07Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHashSupplier.java

@@ -30,7 +33,8 @@
    private final PagesHash pagesHash;
    private final LongArrayList addresses;
    private final List<List<Block>> channels;


most of these fields are no longer used

after fixing thread safety of FilterFunction they are now used again

dain · 2017-04-01T01:31:57Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+    private final PositionLinks positionLinks;
+    private final int[][] sortedPositionLinks;
+
+    private JoinFilterFunction filterFunction;


this can be final

dain · 2017-04-01T01:33:23Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortExpressionExtractor.java

+ * would allow us to perform binary search on arbitrary complex expressions
+ * by sorting position links according to the result of f(...) function.
+ */
+public class SortExpressionExtractor


This is a utility class (all static methods), so it should be final

dain · 2017-04-01T01:56:42Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortExpressionExtractor.java

+    {
+    }
+
+    public static Optional<SortExpression> getSortOrder(Map<Symbol, Integer> buildLayout, Expression filter)


How about extractSortOrder?

actually it should be extractSortExpression. SortOrder was a remanent after a refactor/rename.

dain · 2017-04-01T01:57:28Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

@@ -1600,6 +1601,8 @@ private JoinFilterFunctionFactory compileJoinFilterFunction(

            Expression rewrittenFilter = new SymbolToInputRewriter(joinSourcesLayout).rewrite(filterExpression);

+            Optional<SortExpression> sortChannel = SortExpressionExtractor.getSortOrder(buildLayout, rewrittenFilter);


Can we add a kill switch for this feature incase we find a bug?

pnowojski

I have applied changes, please check my responses to some of your comments.

I have also merged some of those commits, but IMO it's much harder to review one 1000 commit from 1200 lines PR. Indeed, smaller commits are often missing some greater context, but in such case if I have doubts I like to fallback to diff of all commits by whole PR to grasp what PR is about or to search for method usages.

On the other hand smaller commits allows for bundling some changes that belong to some topic. Like in this PR commit "Add compare method to PagesHashStrategy". It would just make unnecessary noise if squashed with other commits.

pnowojski · 2017-04-06T06:25:30Z

presto-benchmark/src/test/java/com/facebook/presto/benchmark/MemoryLocalQueryRunner.java

+        localQueryRunner.createCatalog(
+                "memory",
+                new MemoryConnectorFactory(),
+                ImmutableMap.<String, String>of("max-data-per-node", "4GB"));


fixed, Intellij was auto folding this so I missed it

pnowojski · 2017-04-06T08:42:23Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayPositionLinks.java

+        }
+
+        @Override
+        public void setFilterFunction(JoinFilterFunction filterFunction)


I have moved filterFunction to SortedPositionLinks constructor and dropped it from ArrayPositionLinks.

pnowojski · 2017-04-06T08:54:07Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+            }
+
+            // make sure that from value is the smaller one
+            if (comparator.compare(from, to) > 0) {


Thanks, indeed much better to understand.

pnowojski · 2017-04-06T09:49:39Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+{
+    public static class Builder implements PositionLinks.Builder
+    {
+        private final Map<Integer, List<Integer>> positionLinks;


I have replaced it and didn't notice performance improvement in benchmarks. However code is almost the same, so fastutil version can stay.

pnowojski · 2017-04-06T10:09:13Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+        if (offset < 0) {
+            return -1;
+        }
+        if (!applyFilterFilterFunction(startingPosition, offset, probePosition, allProbeChannelsPage)) {


that's not the only place that uses this assumption. This lines uses it to terminate iteration over position links. Same assumption is used to find a starting point of iteration.

I have added javadoc comment at the top of this class.

pnowojski · 2017-04-06T10:16:29Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+    }
+
+    /**
+     * Find the first element in position links that is NOT smaller than probePosition


I have added some explanation to the java doc, because I can not come up with an idea for better naming. Keep in mind that smaller and larger are defined using an external comparator, which is a transformation of the filterFunction

pnowojski · 2017-04-06T10:18:11Z

presto-main/src/main/java/com/facebook/presto/operator/SortedPositionLinks.java

+    @Override
+    public long getSizeInBytes()
+    {
+        return 0;


wow, nice bug... It must have slipped through since it was just a poc...

pnowojski · 2017-04-06T10:26:57Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SortExpressionExtractor.java

+    {
+    }
+
+    public static Optional<SortExpression> getSortOrder(Map<Symbol, Integer> buildLayout, Expression filter)


actually it should be extractSortExpression. SortOrder was a remanent after a refactor/rename.

pnowojski · 2017-04-06T10:41:23Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

+    public void testJoinWithLessThanInJoinClause()
+            throws Exception
+    {
+        assertQuery("SELECT n.nationkey, r.regionkey FROM region r, nation n WHERE n.regionkey = r.regionkey AND n.name < r.name");


They execute faster in H2 then in presto.

pnowojski · 2017-04-06T13:14:10Z

presto-main/src/main/java/com/facebook/presto/operator/JoinHashSupplier.java

@@ -67,7 +84,33 @@ public double getExpectedHashCollisions()
    @Override
    public JoinHash get()
    {
-        Optional<JoinFilterFunction> filterFunction = filterFunctionFactory.map(factory -> factory.create(session, addresses, channels));


Grrr... :( This makes it more complicated. I can not build positionLinks in this get method, because it would be too costly to sort position links every time get is called.

But on the other hand, I can not return positionLinks created in the constructor with it's private final filterFunction here, because then we hit the same problem you are describing.

The only solution that I can see is to build/sort positionLinks in the constructor with temporarily created filterFunction, but before returning it in in get, this filter function must be overwritten by newly created one.

kokosing · 2017-04-12T21:01:32Z

@dain I have just approved. Thanks for notification ;)

pnowojski · 2017-04-14T08:19:43Z

Applied comments.

I have added some more benchmark cases to see if there is a performance drop for very short position links (larger number of buckets) and surprising I haven't seen that. So indeed I think we can turn this on by default.

Here are the results. Number of elements in build table is 60175, so position links length is equal to 60175 / buckets. Of course the larger position links chain, the better case for the binary search.

Benchmark                              (buckets)  (fastInequalityJoin)  (filterOutCoefficient)  Mode  Cnt     Score    Error  Units
BenchmarkInequalityJoin.benchmarkJoin        100                   true                      10  avgt   30    79.657 ±  4.545  ms/op
BenchmarkInequalityJoin.benchmarkJoin        100                  false                      10  avgt   30  1367.266 ± 28.193  ms/op
BenchmarkInequalityJoin.benchmarkJoin       1000                   true                      10  avgt   30    70.507 ±  4.654  ms/op
BenchmarkInequalityJoin.benchmarkJoin       1000                  false                      10  avgt   30   204.847 ±  6.114  ms/op
BenchmarkInequalityJoin.benchmarkJoin      10000                   true                      10  avgt   30    65.660 ±  4.196  ms/op
BenchmarkInequalityJoin.benchmarkJoin      10000                  false                      10  avgt   30    82.278 ±  3.722  ms/op
BenchmarkInequalityJoin.benchmarkJoin      60000                   true                      10  avgt   30    61.385 ±  3.970  ms/op
BenchmarkInequalityJoin.benchmarkJoin      60000                  false                      10  avgt   30    65.466 ±  5.425  ms/op

Collect output of the memory benchmarks instead of ignoring it

dain · 2017-04-14T16:26:25Z

The "performance drop" you are seeing is in the margin of error, so it doesn't mean anything.

pnowojski · 2017-04-14T17:44:58Z

Maybe I wasn't clear enough. I meant, that I expected to see some performance regression in some edge cases, but it wasn't visible in benchmarks (which is good :) )

dain · 2017-04-14T21:37:56Z

Merged, thanks!

pnowojski · 2017-04-15T09:10:15Z

Thanks :)

pnowojski added draft enhancement labels Jan 16, 2017

facebook-github-bot added the CLA Signed label Jan 16, 2017

losipiuk reviewed Jan 16, 2017

View reviewed changes

kokosing suggested changes Jan 17, 2017

View reviewed changes

pnowojski force-pushed the sort-fb branch 3 times, most recently from caa99ae to b1581d2 Compare January 19, 2017 11:54

pnowojski force-pushed the sort-fb branch 2 times, most recently from fe2d411 to 086735a Compare March 27, 2017 11:48

pnowojski assigned dain Mar 27, 2017

pnowojski force-pushed the sort-fb branch 2 times, most recently from 2fa1cd0 to 1ebf962 Compare March 27, 2017 12:29

losipiuk reviewed Mar 28, 2017

View reviewed changes

pnowojski force-pushed the sort-fb branch from 1ebf962 to de4ef6e Compare March 30, 2017 12:30

pnowojski removed the draft label Mar 30, 2017

pnowojski force-pushed the sort-fb branch 2 times, most recently from 131a7e0 to 3533940 Compare March 31, 2017 09:33

dain suggested changes Apr 1, 2017

View reviewed changes

dain assigned pnowojski and unassigned dain Apr 1, 2017

pnowojski commented Apr 6, 2017

View reviewed changes

pnowojski force-pushed the sort-fb branch 3 times, most recently from f29c054 to 35f85e5 Compare April 7, 2017 08:10

pnowojski removed their assignment Apr 7, 2017

kokosing approved these changes Apr 12, 2017

View reviewed changes

Increase memory limits for memory benchmarks

95cdaab

pnowojski force-pushed the sort-fb branch from 35f85e5 to f08094e Compare April 14, 2017 08:18

pnowojski assigned dain and unassigned kokosing and pnowojski Apr 14, 2017

pnowojski added 10 commits April 14, 2017 10:48

Fix benchmarks using MemoryLocalQueryRunner

d479ddb

Collect output of the memory benchmarks instead of ignoring it

Handle inequality join conditions in PositionLinks

bf43ac8

Implement SortExpressionExtractor

a68b4ad

Add tests queries for sorting position links

32d55ad

Extract sort channel from filter function

640b1bc

Add compare method to PagesHashStrategy

5106aac

Use SortedPositionLinks in LookupJoinOperator

df71be7

Overwrite session properties in memory benchmarks

589bf49

Add inequality join benchmarks

7979af5

Mark JoinFilterFunction not thread safe

3e384e5

pnowojski force-pushed the sort-fb branch from f08094e to 3e384e5 Compare April 14, 2017 09:01

dain approved these changes Apr 14, 2017

View reviewed changes

dain added the ready-to-merge label Apr 14, 2017

dain closed this Apr 14, 2017

dain merged commit 3e384e5 into prestodb:master Apr 14, 2017

pnowojski mentioned this pull request May 17, 2017

Invalid results when fast_inequality_join=true and table contains null values #8025

Closed

anusudarsan mentioned this pull request Jul 18, 2017

Extend fast inequality join Teradata/presto#630

Closed

anusudarsan mentioned this pull request Jul 27, 2017

Extend fast inequality join #8614

Merged


		import static io.airlift.slice.SizeOf.sizeOf;

		public final class PositionLinksList

		@@ -1600,6 +1601,8 @@ private JoinFilterFunctionFactory compileJoinFilterFunction(

		Expression rewrittenFilter = new SymbolToInputRewriter(joinSourcesLayout).rewrite(filterExpression);

		Optional<SortExpression> sortChannel = SortExpressionExtractor.getSortOrder(buildLayout, rewrittenFilter);

Sort position links for faster non-equi joins #7097

Sort position links for faster non-equi joins #7097

Conversation

pnowojski commented Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski commented Mar 27, 2017

dain commented Mar 27, 2017

losipiuk commented Mar 27, 2017

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dain commented Mar 31, 2017 • edited Loading

dain left a comment

Choose a reason for hiding this comment

Fix benchmarks using MemoryLocalQueryRunner

Increase memory limits for benchmarks using MemoryLocalQueryRunner

Add inequality join benchmarks

Add direct handling of inequality conditions in JoinHash

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski commented Jan 16, 2017 •

edited

Loading

pnowojski Jan 19, 2017 •

edited

Loading

dain commented Mar 31, 2017 •

edited

Loading