Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache dictionaries lru cache #20595

Merged
merged 40 commits into from
Mar 9, 2021

Conversation

kitaisreal
Copy link
Collaborator

@kitaisreal kitaisreal commented Feb 16, 2021

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

Fixes #20252.
Fixes #20194.
Fixes #10863.
Fixes #19714.
Fixes #21517.

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added async update in ComplexKeyCache, SSDCache, SSDComplexKeyCache dictionaries.
Added support for Nullable type in Cache, ComplexKeyCache, SSDCache, SSDComplexKeyCache dictionaries.
Added support for multiple attributes fetch with dictGet, dictGetOrDefault functions. Fixes #21517.

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Feb 16, 2021
@nikitamikhaylov nikitamikhaylov self-assigned this Feb 16, 2021
@kitaisreal kitaisreal force-pushed the cache-dictionaries-lru-cache branch 2 times, most recently from f49d8a2 to afbcbae Compare February 18, 2021 18:39
@kitaisreal kitaisreal added the force tests The label does nothing, NOOP, None, nil label Feb 18, 2021
@kitaisreal kitaisreal force-pushed the cache-dictionaries-lru-cache branch 4 times, most recently from aa56276 to 24070a3 Compare February 25, 2021 18:43
@robot-clickhouse robot-clickhouse added the submodule changed At least one submodule changed in this PR. label Feb 27, 2021
@kitaisreal kitaisreal force-pushed the cache-dictionaries-lru-cache branch 2 times, most recently from 449fecc to 712e9f2 Compare March 3, 2021 19:39
const char * ColumnString::skipSerializedInArena(const char * pos) const
{
const size_t string_size = unalignedLoad<size_t>(pos);
pos += sizeof(string_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(size_t) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no difference.

@@ -195,6 +202,8 @@ inline size_t DefaultHash64(std::enable_if_t<(sizeof(T) > sizeof(UInt64)), T> ke
static_cast<UInt64>(key >> 128) ^
static_cast<UInt64>(key >> 256));
}

assert(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw Exception("Wrong specialization for type ...", ErrorCodes::..);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No exception is necessary here, we can't get here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is better for future readers. At least a comment required here

@@ -341,6 +350,11 @@ struct IntHash32
}
else if constexpr (sizeof(T) <= sizeof(UInt64))
return intHash32<salt>(key);

assert(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw Exception("Wrong specialization for type ...", ErrorCodes::..);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No exception is necessary here, we can't get here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is better for future readers. At least a comment required here

}
catch (...)
{
/// TODO: Write log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangerous, it will may lead to std::terminate, because we have ThreadPool

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikitamikhaylov it was your comment.

}
catch (...)
{
std::unique_lock<std::mutex> lock(update_mutex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock_guard

using KeyType = std::conditional_t<dictionary_key_type == DictionaryKeyType::simple, UInt64, StringRef>;

/// Constructor for complex keys update request
explicit CacheDictionaryUpdateUnit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no case when client will want to construct CacheDictionaryUpdateUnit without specifying it type.


auto type_call = [&](const auto &dictionary_attribute_type)
/// TODO: Move this to DictionaryStructure
for (const auto & attribute : dict_struct.attributes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is for next refactoring pull requests related to hierarchy handling.

@kitaisreal kitaisreal force-pushed the cache-dictionaries-lru-cache branch from 7e0c0b2 to 4db7131 Compare March 6, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force tests The label does nothing, NOOP, None, nil pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR.
Projects
None yet
3 participants