SpaceSaving remove last element from map fix#17845
SpaceSaving remove last element from map fix#17845alexey-milovidov merged 14 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-bugfix
kitaisreal
force-pushed
the
space-saving-remove-last-element-from-map-fix
branch
2 times, most recently
from
32c7e22 to
a053e80
Compare
kitaisreal
force-pushed
the
space-saving-remove-last-element-from-map-fix
branch
from
a053e80 to
b70b98c
Compare
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
15061972
00453_top_k.reference
The reference file is too large.
You can wrap topK calls with length.
| #include <Common/HashTable/HashSet.h> | ||
|
|
||
|
|
||
| int main(int, char **) |
There was a problem hiding this comment.
BTW, it's better to "unitize" this test (see gtest_*.cpp files), so it will be run in CI.
src/Common/tests/hash_table.cpp
Outdated
| @@ -1,5 +1,5 @@ | |||
| #include <iostream> | |||
| #include <iomanip> | |||
There was a problem hiding this comment.
BTW, this test is also not unitized (as it is very old), it will be better to convert it to unit test.
src/Common/HashTable/HashTable.h
Outdated
| /// We need to guarantee loop termination because there will be empty position | ||
| assert(m_size < grower.bufSize()); | ||
|
|
||
| size_t j = i; |
There was a problem hiding this comment.
i, j, k are cryptic names, maybe invent some better names?
src/Common/HashTable/HashTable.h
Outdated
|
|
||
| size_t k = grower.place(buf[j].getHash(*this)); | ||
|
|
||
| if (i <= j ? ((k <= i) || (k > j)) : ((k <= i) && (k > j))) |
There was a problem hiding this comment.
memcpy implies i != j but here we check for i <= j that is confusing.
There was a problem hiding this comment.
It's strange to use ?: operator inside if condition.
&& and || will look better.
|
TSan report from the new TestKeeper, @alesapin: |
Backport #17845 to 20.12: SpaceSaving remove last element from map fix
Backport #17845 to 20.11: SpaceSaving remove last element from map fix
Backport #17845 to 20.9: SpaceSaving remove last element from map fix
Backport #17845 to 20.10: SpaceSaving remove last element from map fix
Backport #17845 to 20.8: SpaceSaving remove last element from map fix
3 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Mentioned in #17404.
Changelog category (leave one):
Changelog Entry:
Fix possible segfault in
topKaggregate function. This closes #17404.