Make NULLs comparable to everything.#8834
Make NULLs comparable to everything.#8834alexey-milovidov merged 3 commits intoClickHouse:masterfrom
Conversation
If it is unused right now, better not introduce this tweak at all. |
dbms/src/Common/FieldVisitors.h
Outdated
There was a problem hiding this comment.
Also look at line 342, maybe it was incorrect.
template <typename T>
bool operator() (const Null &, const T &) const
There was a problem hiding this comment.
Yes, I forgot about this method, thanks.
zlobober
force-pushed
the
comparable_nulls
branch
from
9c93e33 to
803760a
Compare
zlobober
left a comment
zlobober
left a comment
There was a problem hiding this comment.
We discussed introduction of weak symbol NULLS_LAST internally with @alexey-milovidov and decided to continue with NULLS_FIRST comparison here without any possible customization.
zlobober
force-pushed
the
comparable_nulls
branch
from
803760a to
af7e66f
Compare
|
@alexey-milovidov, would you mind checking again? |
dbms/src/Common/FieldVisitors.h
Outdated
| return l < r; | ||
| else if constexpr (std::is_same_v<U, Int64> || std::is_same_v<U, UInt64>) | ||
| return l < DecimalField<Decimal128>(r, 0); | ||
| if constexpr (std::is_same_v<T, Null>) |
There was a problem hiding this comment.
:( You are right, I misinterpreted the meaning of T.
|
Request for the next PR:
|
|
One more attempt. |
|
Hey @alexey-milovidov, is your solution implemented? If so, who take it on the instance!? |
|
It seems nullable keys are valid after this pr. I'll help finish what @alexey-milovidov proposed |
|
Ok. And please add a setting to alllow/disallow Nullable in primary key, because allowing them contradicts with SQL standard or usual behaviour of other DBMS. |
5 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
In this PR we make NULL fields comparable to any other fields. Default comparison order is similar to NULLS_LAST, but it may be overridden during linkage by rewriting extern const bool DB::NULLS_LAST weak symbol.
This is important to third-party applications using ClickHouse as a library which rely on DB::KeyCondition in order to prune input possibly containing NULLs.
Changelog category (leave one):
Related: #5319