-
Notifications
You must be signed in to change notification settings - Fork 18
Export partition - Positional column matching might lead to incorrect partitioning #2134
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
base: antalya-26.3
Are you sure you want to change the base?
Changes from all commits
9209361
1a6d63c
420f2e8
5322e48
cb859eb
faef463
31116d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,10 +9,14 @@ | |
| #include <Storages/MergeTree/MergeTreeData.h> | ||
| #include <filesystem> | ||
| #include <thread> | ||
| #include <unordered_map> | ||
| #include <unordered_set> | ||
| #include <Core/Block.h> | ||
| #include <Core/Settings.h> | ||
| #include <DataTypes/DataTypeDateTime.h> | ||
| #include <DataTypes/DataTypeDateTime64.h> | ||
| #include <DataTypes/Utils.h> | ||
| #include <Functions/FunctionHelpers.h> | ||
| #include <Interpreters/ActionsDAG.h> | ||
| #include <Interpreters/Context.h> | ||
| #include <Interpreters/ExpressionActions.h> | ||
|
|
@@ -634,6 +638,32 @@ namespace ExportPartitionUtils | |
| } | ||
| #endif | ||
|
|
||
| namespace | ||
| { | ||
| std::optional<String> getDateTimeTimeZoneName(const DataTypePtr & type) | ||
| { | ||
| if (const auto * datetime_type = checkAndGetDataType<DataTypeDateTime>(type.get())) | ||
| return datetime_type->getTimeZone().getTimeZone(); | ||
| if (const auto * datetime64_type = checkAndGetDataType<DataTypeDateTime64>(type.get())) | ||
| return datetime64_type->getTimeZone().getTimeZone(); | ||
| return {}; | ||
| } | ||
| } | ||
|
|
||
| void verifyMergeTreePartitionCompatibility( | ||
| const StorageMetadataPtr & source_metadata, | ||
| const StorageMetadataPtr & destination_metadata) | ||
| { | ||
| constexpr auto query_to_string = [] (const ASTPtr & ast) | ||
| { | ||
| return ast ? ast->formatWithSecretsOneLine() : ""; | ||
| }; | ||
|
|
||
| if (query_to_string(source_metadata->getPartitionKeyAST()) != query_to_string(destination_metadata->getPartitionKeyAST())) | ||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "Cannot export partition: source and destination tables have different `PARTITION BY` expressions"); | ||
| } | ||
|
|
||
| void verifyExportSchemaCastable( | ||
| const StorageMetadataPtr & source_metadata, | ||
| const StorageMetadataPtr & destination_metadata, | ||
|
|
@@ -657,15 +687,51 @@ namespace ExportPartitionUtils | |
| ActionsDAG::MatchColumnsMode::Position, | ||
| context); | ||
|
|
||
| /// Lossy casts may silently change values, so reject them unless the user opts in. | ||
| if (context->getSettingsRef()[Setting::export_merge_tree_part_allow_lossy_cast]) | ||
| return; | ||
| auto partition_key_columns = source_metadata->getColumnsRequiredForPartitionKey(); | ||
| const std::unordered_set<String> partition_key_column_set( | ||
| std::make_move_iterator(partition_key_columns.begin()), | ||
| std::make_move_iterator(partition_key_columns.end())); | ||
|
Comment on lines
+690
to
+693
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
|
|
||
| const bool allow_lossy_cast = context->getSettingsRef()[Setting::export_merge_tree_part_allow_lossy_cast]; | ||
|
|
||
| const size_t num_columns = std::min(source_columns.size(), destination_columns.size()); | ||
| for (size_t i = 0; i < num_columns; ++i) | ||
| { | ||
| const auto & source_column = source_columns[i]; | ||
| const auto & destination_column = destination_columns[i]; | ||
|
|
||
| if (partition_key_column_set.contains(source_column.name) && source_column.name != destination_column.name) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if partitioning is for several columns, and destination has same columns but in different order? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or when destination has more columns in partition list than source.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the tests for simular cases:
|
||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "Cannot export to {}: partition key column '{}' is at position {} in the source " | ||
| "table, but the destination's column at that position is named '{}'. EXPORT " | ||
| "PART/PARTITION matches columns by position, so partition key columns must be " | ||
| "declared at the same position in both tables.", | ||
| destination_storage_id.getFullTableName(), | ||
| source_column.name, | ||
| i, | ||
| destination_column.name); | ||
|
|
||
| if (partition_key_column_set.contains(source_column.name)) | ||
| { | ||
| const auto source_time_zone = getDateTimeTimeZoneName(source_column.type); | ||
| const auto destination_time_zone = getDateTimeTimeZoneName(destination_column.type); | ||
| if (source_time_zone && destination_time_zone && *source_time_zone != *destination_time_zone) | ||
| throw Exception(ErrorCodes::BAD_ARGUMENTS, | ||
| "Cannot export to {}: partition key column '{}' is {} in the source table " | ||
| "but {} in the destination. The destination's hive-style partition path is " | ||
| "rendered from the source value without converting the timezone, so this " | ||
| "would silently shift the exported value by the timezone offset. Use the " | ||
| "same timezone in both tables' partition key column.", | ||
| destination_storage_id.getFullTableName(), | ||
| destination_column.name, | ||
| source_column.type->getName(), | ||
| destination_column.type->getName()); | ||
| } | ||
|
|
||
| /// Lossy casts may silently change values, so reject them unless the user opts in. | ||
| if (allow_lossy_cast) | ||
| continue; | ||
|
|
||
| if (!canBeSafelyCast(source_column.type, destination_column.type)) | ||
| throw Exception(ErrorCodes::INCOMPATIBLE_COLUMNS, | ||
| "Cannot export to {}: column '{}' requires a lossy cast from {} to {}, " | ||
|
|
||
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 found method
getExplicitTimeZoneOfDateTimeArgumentlike this.And fragment from method
extractTimeZoneNameFromFunctionArguments.May be possible to reuse something?
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 they are a slightly defferent. But I could use
checkAndGetDataTypefrom them.