Request for feedback on implementation of ASOF join

[Gladdy]

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog.

• Not meant for merging yet, merely a request for feedback at this point as I do not have a huge amount of experience with the Clickhouse codebase.

Category

• New Feature

Short description
I implemented an asof join strictness to the already existing All and Any. This allows you to run queries that join to the most recent value known. The API idea is based on the kdb+ asof operator (https://code.kx.com/q/ref/joins/#aj-aj0-ajf-ajf0-asof-join), using the last column specified as the asof column that would represent the timestamp. Note, that this is a proof of concept implementation looking for some feedback on whether this would be mergeable at some point or is fundamentally flawed. Therefore, there are still some limitations on the functionality (see below).

Detailed description
The asof strictness would allow you to run queries like these:

CREATE TABLE A(k UInt32, t DateTime, a Float64) ENGINE = MergeTree() ORDER BY (k, t);
INSERT INTO A(k,t,a) VALUES (1,1,1),(1,2,2),(1,3,3),(1,4,4),(1,5,5),(2,1,1),(2,2,2),(2,3,3),(2,4,4),(2,5,5);
CREATE TABLE B(k UInt32, t DateTime, b Float64) ENGINE = MergeTree() ORDER BY (k, t);
INSERT INTO B(k,t,b) VALUES (1,2,2),(1,4,4), (2,3,3);
SELECT A.k, A.t, A.a, B.b, B.t, B.k FROM A ASOF LEFT JOIN B USING(k,t) ORDER BY (A.k, A.t);

k                     t   a   b                   B.t   B.k

1   1970-01-01 01:00:01   1   0   0000-00-00 00:00:00     0
1   1970-01-01 01:00:02   2   2   1970-01-01 01:00:02     1
1   1970-01-01 01:00:03   3   2   1970-01-01 01:00:03     1
1   1970-01-01 01:00:04   4   4   1970-01-01 01:00:04     1
1   1970-01-01 01:00:05   5   4   1970-01-01 01:00:05     1
2   1970-01-01 01:00:01   1   0   0000-00-00 00:00:00     0
2   1970-01-01 01:00:02   2   0   0000-00-00 00:00:00     0
2   1970-01-01 01:00:03   3   3   1970-01-01 01:00:03     2
2   1970-01-01 01:00:04   4   3   1970-01-01 01:00:04     2
2   1970-01-01 01:00:05   5   3   1970-01-01 01:00:05     2

Current limitations on the implementation:

1. The ASOF strictness only works in combination with a LEFT join (intended for the main use case of joining a high frequency time series on the left to a lower frequency one on the right)
2. The only type supported for the ASOF timestamp column is UInt32 (DateTime)
3. The ASOF join is not supported in the StorageJoin
4. The ASOF join column cannot appear alone, as that would essentially make it a cross join which takes a different internal code path and the asof logic hasn't been plumbed through there.

Stuff that should be improved before merging:

1. Internally, for a normal join it creates a hash_table of (key_1, key_2, key_n) to a RowRef. For now, the asof join it creates a hash table of (key_1, key_2, ..., key_n-1) to a std::map<key_n, RowRef>. This std::map should really be using the Arena allocator already available in the Join object, but to get some feasibility feedback this hasn't been done yet (as also the allocator api for Arena does not match std::allocator, hence a custom binary search tree would have to be written).
2. Fix the bug that returns the wrong timestamp for the low frequency column (see query result above)
3. Depending on feedback, some of the limitations should be addressed.
4. ?? Please let me know if you have any further ideas.

[alexey-milovidov]

@KochetovNicolai briefly reviewed and said that the idea is all right. I will look further in a moment.

[Gladdy]

@KochetovNicolai briefly reviewed and said that the idea is all right. I will look further in a moment.

@alexey-milovidov Thanks very much! If you have any further feedback/issues I might have overlooked that need to be addressed before any any possibility of merging, please let me know before I put in some more work to address the current issues. I looked over the failed tests and other than the glibc compatibility and performance checks (the failing ones do not seem to be related to joining tables?) they seem to be straightforward to fix from here.

Furthermore, I had a quick look through the codebase looking for binary-tree like structure that is Arena allocator enabled, but wasn't able to find any. Is it correct that clickhouse doesn't use any, eg. for lookups into indexed columns?

[Gladdy]

I've updated the pull request to match the recent refactoring of Join.cpp and my local tests pass again.

There is still some code smell around having to pass in the timestamp, but in order to avoid this without much change to the rest of the codebase it might have to be required to write a specialized HashMap that includes a binary search tree as the last level lookup. It would have to be aware of the asof criterion and would not fit nicely into the existing MapsTemplate in Join.h.

[Gladdy]

@4ertus2 Thanks very much for your time in leaving some very useful feedback! I'll reply to the individual points inline and try to fix them up as well as possible tonight.

[Gladdy]

@4ertus2 The current approach introduces quite a lot of if constexprs into the Join class - what is your option on rather than having these conditionals, templating in different implementations of the core HashTable (similar-ish to the 2-stage hash table, but then with a normal HT at the base followed by a BST for the final layer) which internally does the asof join and implements the same API as HashTable. It would introduce quite a bit more code in the implementation and also somehow the KeyGetterForTypeImpls would need to be aware of it as the Key type now needs to index into both the HashTable and the BST, but it would clean up the Join impl quite a bit. What are your thoughts on this?

[4ertus2]

@4ertus2 The current approach introduces quite a lot of if constexprs into the Join class - what is your option on rather than having these conditionals, templating in different implementations of the core HashTable (similar-ish to the 2-stage hash table, but then with a normal HT at the base followed by a BST for the final layer) which internally does the asof join and implements the same API as HashTable. It would introduce quite a bit more code in the implementation and also somehow the KeyGetterForTypeImpls would need to be aware of it as the Key type now needs to index into both the HashTable and the BST, but it would clean up the Join impl quite a bit. What are your thoughts on this?

My opinion is, it would be cool to merge current version and make a new PR to discuss improvements. I really want to test your ASOF JOIN on my data these holidays %)

[Gladdy]

I think your CI might have some issues - it's reporting the error below for commit e7a10b8 (https://clickhouse-builds.s3.yandex.net/4774/e7a10b8a3e82b7abef2b62db70ae219b16007fa7/build_log_403388044_1553802981.txt)

../dbms/src/Interpreters/Join.cpp:1417:123: error: too few arguments to function call, expected 4, have 3
            AdderNonJoined<STRICTNESS, typename Map::mapped_type>::add(it->getSecond(), rows_added, columns_keys_and_right);
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  

, but that that call isn't actually present in Join.cpp for that commit hash (nor is it the right line where the call would be, with the right number of arguments).

[4ertus2]

I think your CI might have some issues - it's reporting the error below for commit e7a10b8 (https://clickhouse-builds.s3.yandex.net/4774/e7a10b8a3e82b7abef2b62db70ae219b16007fa7/build_log_403388044_1553802981.txt)

../dbms/src/Interpreters/Join.cpp:1417:123: error: too few arguments to function call, expected 4, have 3
            AdderNonJoined<STRICTNESS, typename Map::mapped_type>::add(it->getSecond(), rows_added, columns_keys_and_right);
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  

, but that that call isn't actually present in Join.cpp for that commit hash (nor is it the right line where the call would be, with the right number of arguments).

Try to merge master to your branch. I've recently changed some NonJoinedBlockInputStream logic. It probably gets build cache from master branch.

P.S. We have discussed it. It's an expected behaviour that it would not build if it cannot be merged into master without errors.

[Gladdy]

@4ertus2 Thanks very much, have merged master and am fixing the issues locally before re-pushing.

[4ertus2]

Furthermore, I had a quick look through the codebase looking for binary-tree like structure that is Arena allocator enabled, but wasn't able to find any. Is it correct that clickhouse doesn't use any, eg. for lookups into indexed columns?

We've discussed it with @alexey-milovidov. The right way is to insert data into PODArray, sort it and then use std::lower_bound and std::upper_bound to search.