Refactoring of using ExternalLoader in dictionary DDL. #8055
Conversation
vitlibar
force-pushed
the
more-accurate-using-load-reload
branch
3 times, most recently
from
de72381
to
44ccfbf
Compare
vitlibar
force-pushed
the
more-accurate-using-load-reload
branch
from
44ccfbf
to
2e7a246
Compare
vitlibar
changed the title
Instead of using ExternalLoader::reload() now it's used reloadConfig() which reloads only what necessary. Functions attachDictionary() and detachDictionary() are simplified and have lesser number of parameters. Instead of injecting into LoadablesConfigReader's internals for creating dictionary a temp repository is used.
vitlibar
force-pushed
the
more-accurate-using-load-reload
branch
from
2e7a246
to
4c15700
Compare
| "Dictionary " + full_name + " already exists.", | ||
| ErrorCodes::DICTIONARY_ALREADY_EXISTS); | ||
| if (!dictionaries.emplace(dictionary_name).second) | ||
| throw Exception("Dictionary " + full_name + " already exists.", ErrorCodes::DICTIONARY_ALREADY_EXISTS); |
There was a problem hiding this comment.
createDictionary() adds a temp metadata file with a dictionary to ExternalLoader, then loads the dictionary (if lazy_load == false), then calls attachDictionary(). At the moment when attachDictionary() is called the dictionary could be already loaded. I don't think this check was very useful before anyway.
| @@ -427,7 +427,8 @@ DictionaryConfigurationPtr getDictionaryConfigurationFromAST(const ASTCreateQuer | |||
|
|
|||
| AutoPtr<Poco::XML::Element> name_element(xml_document->createElement("name")); | |||
| current_dictionary->appendChild(name_element); | |||
| AutoPtr<Text> name(xml_document->createTextNode(query.database + "." + query.table)); | |||
| String full_name = (!database_name.empty() ? database_name : query.database) + "." + query.table; | |||
There was a problem hiding this comment.
Seems like it's already substituted in InterpreterCreateQuery https://github.com/clickhouse/ClickHouse/blob/master/dbms/src/Interpreters/InterpreterCreateQuery.cpp#L714.
There was a problem hiding this comment.
It's substituted, but the result of that substitution (i.e. database's name) isn't passed to createDictionary(). Seems it's better to set create.database there than the current solution, I'll simplify the code in a separate PR.
|
|
||
| namespace DB | ||
| { | ||
| /// A config repository filled with preset loadables used by ExternalLoader. |
There was a problem hiding this comment.
I think I'll rename this class to something like ExternalLoaderTempConfigRepository and write a more detailed comment in a separate PR.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category:
Instead of using ExternalLoader::reload() now it's used reloadConfig() which reloads only what necessary. Functions attachDictionary() and detachDictionary() are simplified and have lesser number of parameters. Instead of injecting into LoadablesConfigReader's internals for creating dictionary a temp repository is used.