ASTTableIdentifier part #2: Introduce ASTTableIdentifier#16401
ASTTableIdentifier part #2: Introduce ASTTableIdentifier#16401nikitamikhaylov merged 45 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-other
abyss7
changed the title
|
Maybe reopen later. |
|
@mergify update |
|
Command
Hey, I reacted but my real name is @Mergifyio |
… into ast-table-identifier-2
| fuzz(table_expr->database_and_table_name); | ||
| fuzz(table_expr->subquery); | ||
| fuzz(table_expr->table_function); | ||
| fuzz(table_expr->children); |
There was a problem hiding this comment.
Have you removed the database_and_table_name and table_function fields from ASTTableExpression?
There was a problem hiding this comment.
No, but they are included as children already, so it just simplifies the code.
| { | ||
| auto subquery_it = data.subqueries.find(table_id.table_name); | ||
| /// Clang-tidy is wrong on this line, because `func.arguments->children.at(1)` gets replaced before last use of `name`. | ||
| auto name = identifier->shortName(); // NOLINT |
There was a problem hiding this comment.
Better to point out the warning name such as // NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
| { | ||
| auto & ast_identifier = ast->as<ASTIdentifier &>(); | ||
| if (ast_identifier.children.empty()) | ||
| auto ast_identifier = dynamic_pointer_cast<ASTIdentifier>(ast); |
There was a problem hiding this comment.
typeid_cast or assert_cast?
There was a problem hiding this comment.
It's a shortcut - not to check ASTIdentifier and ASTTableIdentifier with the same code.
|
|
||
| if (!ast_identifier.semantic->special && name_parts.size() >= 2) | ||
| ast_identifier.semantic->table = ast_identifier.name_parts.end()[-2]; | ||
| /// FIXME: what should this mean? |
There was a problem hiding this comment.
I don't know too. Maybe we can delete it and see what will happen in tests?
There was a problem hiding this comment.
I suggest to leave it for the next iteration of refactoring.
| if (ast) | ||
| { | ||
| if (const auto * node = ast->as<ASTIdentifier>()) | ||
| if (const auto * node = dynamic_cast<const ASTIdentifier *>(ast)) |
There was a problem hiding this comment.
It's a shortcut - not to check ASTIdentifier and ASTTableIdentifier with the same code.
| if (s_uuid.ignore(pos, expected)) | ||
| { | ||
| ParserStringLiteral uuid_p; | ||
| ASTPtr ast_uuid; | ||
| if (!uuid_p.parse(pos, ast_uuid, expected)) | ||
| return false; | ||
| uuid = parseFromString<UUID>(ast_uuid->as<ASTLiteral>()->value.get<String>()); | ||
| } | ||
|
|
||
| if (parts.size() == 1) node = std::make_shared<ASTTableIdentifier>(parts[0], std::move(params)); | ||
| else node = std::make_shared<ASTTableIdentifier>(parts[0], parts[1], std::move(params)); | ||
| node->as<ASTTableIdentifier>()->uuid = uuid; | ||
| } | ||
| else | ||
| node = std::make_shared<ASTIdentifier>(std::move(parts), false, std::move(params)); |
There was a problem hiding this comment.
It's parsing of a table identifier - it may have only table name, or both database and table names. Also it may have UUID.
| if (ParserKeyword("LIKE").ignore(pos, expected)) | ||
| { | ||
| if (!ParserCompoundIdentifier(false).parse(pos, like_table, expected)) | ||
| if (!ParserCompoundIdentifier(true).parse(pos, like_table, expected)) |
There was a problem hiding this comment.
Why we change table_name_with_optional_uuid_ ctor parameter to true?
There was a problem hiding this comment.
Because ParserCompoundIdentifier(true) stands for now for ParseTableIdentifier - it returns ASTTableIdentifier and parses it accordingly (i.e. with UUID).
Co-authored-by: Nikita Mikhaylov <[email protected]>
4 participants
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):
Introduce ASTTableIdentifier into the code