Refactor joinGet and implement multi-key lookup.#12418
Refactor joinGet and implement multi-key lookup.#12418alexey-milovidov merged 3 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-improvement
amosbird
force-pushed
the
jgmk
branch
4 times, most recently
from
70339a5 to
14d2984
Compare
|
Irrelevant failure due to test is flaky. |
src/Functions/FunctionJoinGet.h
Outdated
There was a problem hiding this comment.
It means that the 3rd argument is always constant - does not look correct.
Did you mean - first two arguments ({0, 1})?
There was a problem hiding this comment.
Yeah, I'll update...
src/Functions/FunctionJoinGet.h
Outdated
There was a problem hiding this comment.
getReturnType is supposed to be a private method for overload resolution. The return type can also be directly inferred in method build when Nullable is handled manually. I'll add some comment...
BTW, performance test has failed not because it is flaky but because performance on master has been improved. |
| throw Exception( | ||
| "Number of arguments for function " + getName() + " doesn't match: passed " + toString(number_of_arguments) | ||
| + ", should be greater or equal to 3", | ||
| ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH); |
There was a problem hiding this comment.
I don't see where checkNumberOfArgumentsIfVariadic() is called, in this case it should bail out in getJoin when it access arguments w/o checking, but why the tests passed?
Interesting, it does not SIGSEGV's because there is std::out_of_range exception:
2020.07.15 06:50:15.428892 [ 225276 ] {a1cfa369-a232-4df9-a456-95ee0af99391} executeQuery: (from [::1]:44804) SELECT joinGet();
2020.07.15 06:50:15.429338 [ 225276 ] {a1cfa369-a232-4df9-a456-95ee0af99391} executeQuery: std::exception. Code: 1001, type: std::out_of_range, e.what() = vector (version 20.7.1.4090) (from [::1]:44804) (in query: SELECT joinGet(); )
@akuzm worth converting out_of_range to abort() ? (like in #12522 and related)
There was a problem hiding this comment.
FWIW this is the only std exception in the log:
$ fgrep -a 'type: std::' clickhouse-server.log -c
99(log from this build stateless tests)
4 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Now joinGet supports multi-key lookup.
#12409
Detailed description / Documentation draft:
Actually the current document implies the use case of multi-keys. Nothing needs to be changed.