Antalya-26.3 Frontport of #1659 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks#1723
Antalya-26.3 Frontport of #1659 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks#1723mkmkme wants to merge 2 commits intoantalya-26.3from
Conversation
Antalya 26.1 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks
mkmkme
added
antalya
hybrid
antalya-26.3
forwardport
| const ASTPtr & base_predicate, const std::vector<StorageDistributed::HybridSegment> & segs) | ||
| { | ||
| std::unordered_map<String, String> result; | ||
| std::function<void(const ASTPtr &)> walk = [&](const ASTPtr & node) |
There was a problem hiding this comment.
This function looks very similar to replace_hybrid_params, maybe there is a way to reduce the code duplication?
There was a problem hiding this comment.
I see this function only validates it, which seems to be a copy and paste of replace_hybrid_params without the replace logic. Maybe you can clone the original ast and just call replace? This will also make the maintainance better
| const auto & param_name = name_lit->value.safeGet<String>(); | ||
| const auto & type_name = type_lit->value.safeGet<String>(); | ||
|
|
||
| if (!watermark_snapshot) |
There was a problem hiding this comment.
Can't this be checked on line 1234? Right after auto watermark_snapshot = hybrid_watermark_params.get();
There was a problem hiding this comment.
It can, but in this case we won't have a watermark name to put into the exception message
|
|
||
|
|
||
| /// Extract declared hybridParam types from all Hybrid predicate ASTs. | ||
| static std::unordered_map<String, String> collectHybridParamTypes( |
There was a problem hiding this comment.
Does it really extract types? It seems to me it actually builds a map from the hybrid args, not only type names.
There was a problem hiding this comment.
The map stores {name1: type1, ...} so I don't really see the conflict here. I mean, yeah, it does build a map for the hybrid args, but it's only the mapping from the name to the type.
| if (auto * func = node->as<ASTFunction>(); func && func->name == "hybridParam") | ||
| { | ||
| auto * arg_list = func->arguments ? func->arguments->as<ASTExpressionList>() : nullptr; | ||
| if (!arg_list || arg_list->children.size() != 2) | ||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "hybridParam() requires exactly 2 arguments: (name, type)"); | ||
|
|
||
| auto * name_lit = arg_list->children[0]->as<ASTLiteral>(); | ||
| auto * type_lit = arg_list->children[1]->as<ASTLiteral>(); | ||
| if (!name_lit || name_lit->value.getType() != Field::Types::String) | ||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "hybridParam() first argument (name) must be a string literal"); | ||
| if (!type_lit || type_lit->value.getType() != Field::Types::String) | ||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "hybridParam() second argument (type) must be a string literal"); | ||
|
|
||
| const auto & param_name = name_lit->value.safeGet<String>(); | ||
| const auto & type_name = type_lit->value.safeGet<String>(); |
There was a problem hiding this comment.
IMHO we should abstract away these checks. Searching for type_lit->value.getType() != Field::Types::String shows 3 occurrences.
4 participants
Antalya 26.1 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added support for moving Hybrid table watermarks (#1659 by @mkmkme)
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: