Export partition - Positional column matching might lead to incorrect partitioning - #2134
Export partition - Positional column matching might lead to incorrect partitioning#2134k-morozov wants to merge 6 commits into
Conversation
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
9e63fd6 to
5322e48
Compare
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
k-morozov
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faef463617
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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())); |
There was a problem hiding this comment.
When PARTITION BY references a subcolumn such as t.a, getColumnsRequiredForPartitionKey returns the subcolumn name, while source_columns contains only the top-level readable column t; consequently, this set never matches and the new positional check is skipped. For example, source columns (t Tuple(a UInt32), u Tuple(a UInt32)) and destination columns (u Tuple(a UInt32), t Tuple(a UInt32)), both partitioned by t.a, have positionally compatible types and pass validation, but export source u into destination t while constructing the partition from source t.a, silently mispartitioning the data. Map required subcolumns back to their top-level storage columns before constructing this set.
Useful? React with 👍 / 👎.
| 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.
What if partitioning is for several columns, and destination has same columns but in different order?
Both have columns 'a' and 'b', in same order, but source with partition by a,b, and destination partition by b,a.
There was a problem hiding this comment.
Or when destination has more columns in partition list than source.
There was a problem hiding this comment.
I added the tests for simular cases:
What if partitioning is for several columns, and destination has same columns but in different order?
https://github.com/Altinity/ClickHouse/pull/2134/changes#diff-28d3e30160a3442e2b9ef01e3bf0b10b4c27de6a39efea3175ccec0a3f2d8949R1786
Or when destination has more columns in partition list than source.
https://github.com/Altinity/ClickHouse/pull/2134/changes#diff-28d3e30160a3442e2b9ef01e3bf0b10b4c27de6a39efea3175ccec0a3f2d8949R1799
|
|
||
| namespace | ||
| { | ||
| std::optional<String> getDateTimeTimeZoneName(const DataTypePtr & type) |
There was a problem hiding this comment.
I found method getExplicitTimeZoneOfDateTimeArgument like this.
And fragment from method extractTimeZoneNameFromFunctionArguments.
May be possible to reuse something?
2 participants
Closes: #2123
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed ALTER TABLE ... EXPORT PART / EXPORT PARTITION silently writing values into the wrong destination columns when the source and destination tables declared the same PARTITION BY expression but with the partition key columns at different physical positions in the schema. The export now validates that partition key columns occupy the same position in both tables and is rejected with BAD_ARGUMENTS otherwise.
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: