-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Resolve table dependencies on metadata loading #28373
Conversation
55fdb23
to
c8d8f0a
Compare
fed00a8
to
666a3ae
Compare
Test failures are irrelevant |
@Mergifyio update |
Command
|
Functional stateless tests flaky check (address) - |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good to me, simple and straightforward implementation.
|
||
void DDLDependencyVisitor::visit(const ASTFunction & function, Data & data) | ||
{ | ||
if (function.name == "joinGet" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it worth to make a property for such functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to create a function through FunctionFactory just to check if it can take table/dictionary name as argument or not, so I'm not sure.
if (literal->value.getType() != Field::Types::String) | ||
return; | ||
|
||
String maybe_qualified_name = literal->value.get<String>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still don't have a function for parsing qualified names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we don't, because for historical reasons parsing of qualified name from string literal is a bit different from parsing of compound identifier from SQL query.
else if (const auto * identifier = arg->as<ASTIdentifier>()) | ||
{ | ||
auto table_identifier = identifier->createTable(); | ||
if (!table_identifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment? Not clear to me how we can get here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get here if create query is incorrect and compound identifier has 3 (or more) name parts.
if (!dict_source.elements) | ||
return; | ||
|
||
auto config = getDictionaryConfigurationFromAST(data.create_query->as<ASTCreateQuery &>(), data.global_context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also very soon we will have predefined connections... I'd prefer to move this part into separate function like isLocalDictionaryFromClickHouseTable
or something like this.
class ASTFunctionWithKeyValueArguments; | ||
|
||
|
||
class DDLDependencyVisitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need comment.
src/Databases/TablesLoader.h
Outdated
String default_database; | ||
|
||
std::mutex mutex; | ||
ParsedMetadata metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadata inside metadata :) Maybe just ParsedTables
?
@@ -14,4 +15,6 @@ void loadMetadataSystem(ContextMutablePtr context); | |||
/// Use separate function to load system tables. | |||
void loadMetadata(ContextMutablePtr context, const String & default_database_name = {}); | |||
|
|||
void startupSystemTables(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment why it is required?
iterateMetadataFiles(local_context, process_metadata); | ||
|
||
size_t total_tables = file_names.size() - total_dictionaries; | ||
ParsedTablesMetadata metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metadata -> parsed_tables?
|
||
void startLoadingIndependentTables(ThreadPool & pool, size_t level); | ||
|
||
void checkCyclicDependencies() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be called only after we have loaded everything what was possible to load.
} | ||
|
||
void DatabaseOrdinary::startupTablesImpl(ThreadPool & thread_pool) | ||
void DatabaseOrdinary::startupTables(ThreadPool & thread_pool, bool /*force_restore*/, bool /*force_attach*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly unclear interface -- why we create pool out of this function scope, but wait it here?
Integration tests failures related? |
Seems like they are |
Functional stateless tests (release, DatabaseReplicated) - merge stuck with
|
What do you think of #7944 - needed for schema dumps & recreate on the other node |
/// How many dependencies this table have | ||
size_t dependencies_count = 0; | ||
/// List of tables/dictionaries which depend on this table/dictionary | ||
TableNames dependent_database_objects; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency_kind (enum) may also be useful sometimes (could be exposed in system.tables for introspection / schema analise) but can be done later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Functional stateless tests (release, DatabaseReplicated) - 01161_all_system_tables - fixed in #29061 |
It's because table |
else if (Poco::toLower(function.name) == "in") | ||
{ | ||
extractTableNameFromArgument(function, data, 1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tavplubix
Do you remember what is the reason that loading of a table depends on the default expressions for its columns? The default expression should be required only during insertion to that table, shouldn't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, there was a check that the default expression type is compatible with the column type, but probably there's another reason.
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):
Resolve table dependencies on metadata loading. Closes #8004, closes #15170.
Detailed description / Documentation draft: