Antalya 26.3 allow non matching schema export partition by name - #2220
Antalya 26.3 allow non matching schema export partition by name#2220k-morozov wants to merge 11 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| - In `strict` mode, a different number of source and destination columns throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`. | ||
| - In `ignore_extra_source_columns_by_position` mode, the destination having more columns than the source throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`. | ||
| - In `ignore_extra_source_columns_by_name` mode, when the source has more columns than the destination, a destination column absent from the source (including a renamed one) throws `THERE_IS_NO_COLUMN`; the mode does not fall back to positional matching in that case. | ||
| - In `ignore_extra_source_columns_by_name` mode, when the source does not have more columns than the destination, a column-count mismatch throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`, same as `strict`. |
There was a problem hiding this comment.
Doesn't make sense to allow equal number of columns with ability to reorder? Now it I want to reorder I can add dummy unused column to source table. It's weird.
It's topic to discussion, not to implement in current PR.
There was a problem hiding this comment.
May be in future we add modes to skip some destination columns.
There was a problem hiding this comment.
Doesn't make sense to allow equal number of columns with ability to reorder?
I also agree with you. I think we should re-design this to something like #2220 (comment)
There was a problem hiding this comment.
@ianton-ru I've been thinking about this. First, I added ignore_extra_source_columns_by_name, which also worked for cases where the number of columns matches. But I think this is the wrong semantics - because the setting's name talks about extra columns, and the user expects the setting to work only for cases with extra columns. If it also overrides matching when the number of columns is the same, that would be misleading. So in one of the last commits I dropped this idea - the setting should be clear and intuitive in its behavior. On the other hand, I now dropped matching by name when the number of columns is the same. I think this is useful functionality, but then the settings need to be made differently. Export partition is an experimental feature and we can change them.
My first idea was to extend export_merge_tree_part_schema_mismatch_mode:
strict_py_pos
strict_by_name
ignore_extra_source_columns_by_position
ignore_extra_source_columns_by_name
So the first 2 modes handle the semantics when the number of columns matches, and the last 2 when it doesn't match.
But this setting has downsides - it's very manual, meaning you can't set it in the system as a default for all exports. Then I thought it would make sense to split it into 2 settings: one for when the number of columns matches, and one for when there are extra columns:
export_partition_matching_columns_policy:
position
name
export_partition_matching_extra_columns_policy:
position
name
This gives flexibility - we can do matching differently in these 2 cases, but this is also a source of problems for the user, since they might forget about the 2 settings. And that worried me.
@arthurpassos , below, suggested a 3rd approach: instead of export_partition_matching_extra_columns_policy, add a flag for whether extra columns are allowed at all or not. And I think this is the most user-friendly option.
Let me know what you think - I think the current PR is a good place to use the settings described above instead of the current ones.
There was a problem hiding this comment.
@k-morozov @arthurpassos
What about two settings?
export_merge_tree_part_schema_match_mode with values match_by_position and match_by_name
and separate flag ignore_unmatched_columns? (Or may be ignore_unmatched_source_columns, and add in future ignore_unmatched_destination_columns?
There was a problem hiding this comment.
@ianton-ru I think that's exactly what I suggested in #2220 (comment), isn't it?
There was a problem hiding this comment.
export_partition_matching_columns_policy:
position
name
export_partition_matching_extra_columns_policy:
position
name
I don't see a reason why someone can want to change policy from name to position or vice versa when add a new column in source table. It breaks already exported data.
|
Hm... I am not so sure this is the path we should take. It seems like we need two settings instead of one. It has been a few weeks we discussed this and I was on PTO, but I thought we wanted to implement something like: column_match_mode = ['position', 'name'] column_count_mismatch = true/false The current approach seems to fail when the schemas have the same amount of columns but in different order. For example: What do you think? |
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
374fd8d to
41b9a94
Compare
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b3e0af86c
ℹ️ 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".
| match_by_name | ||
| ? ActionsDAG::MatchColumnsMode::Name | ||
| : ActionsDAG::MatchColumnsMode::Position, |
There was a problem hiding this comment.
Recheck extra columns in the background worker
When match_by_name is used with export_merge_tree_part_ignore_extra_source_columns = false, a destination column removed after request-time validation but before the background task runs is silently dropped from the export. ActionsDAG::MatchColumnsMode::Name explicitly permits excess source columns, and this worker only trims/checks extras in positional mode, so the strict setting is no longer enforced against the destination's current schema. Reject unmatched source columns here (or rerun schema validation) before constructing the name-matching DAG.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good comment. This is most likely a problem with the current export design. I'll add tests to reproduce the issue and a couple of checks to ensure the DAG is built correctly. This still doesn't completely solve the problem, but for the purposes of PR, it's sufficient.
There was a problem hiding this comment.
and this worker only trims/checks extras in positional mode
why tho? I am a bit confused on this one tbh
There was a problem hiding this comment.
This is how makeConvertingActions works.
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
arthurpassos
left a comment
There was a problem hiding this comment.
I have not reviewed the integration tests yet
| { | ||
| strict, | ||
| ignore_extra_source_columns_by_position, | ||
| match_by_position, |
There was a problem hiding this comment.
I think the values should be:
POSITION and NAME. No need to repeat the match keyword and use underscores
| The following must match between source and destination: | ||
|
|
||
| 1. **Column count** - source and destination must have the same number of columns by default. A mismatch in either direction throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`. Set `export_merge_tree_part_schema_mismatch_mode = 'ignore_extra_source_columns_by_position'` to allow a source table with extra trailing columns; the destination having more columns than the source is still rejected in this mode. | ||
| 1. **Column count** - by default (`export_merge_tree_part_ignore_extra_source_columns = false`) every source column must have a corresponding destination column: with `export_merge_tree_part_schema_match_mode = 'match_by_position'` (the default) the source and destination must have the same number of columns; with `'match_by_name'` they must have the same set of column names. A mismatch throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`. Set `export_merge_tree_part_ignore_extra_source_columns = 1` to allow a source table with columns that have no corresponding destination column; such columns are dropped and not exported. The destination having a column absent from the source is always rejected, regardless of this setting. |
There was a problem hiding this comment.
The writing is slightly confusing. It says
every column must have a corresponding destination column: with
export_merge_tree_part_schema_match_mode = 'match_by_position'and destination must have the same number of columns;
This gives the idea that matching by name does not require this, which is not true. I would rephrase it something along the following lines:
By default, source and destination must have the same number of columns. This requirement can be relaxed through
ignore_extra_columns.... Columns are matched according to the policy specified inmatch_by_setting...
| - **Type**: `Bool` | ||
| - **Default**: `false` | ||
| - **Description**: Controls whether `EXPORT PART`/`EXPORT PARTITION` tolerates source `MergeTree` columns that have no corresponding destination column. | ||
| - `false` (default) - such a source column is rejected: the source and destination must match exactly. With `export_merge_tree_part_schema_match_mode = 'match_by_position'` this means the same number of columns; with `'match_by_name'` this means the same set of column names, so a source table with columns absent from the destination is rejected even if the matched columns would otherwise be compatible. A mismatch throws `NUMBER_OF_COLUMNS_DOESNT_MATCH`. |
There was a problem hiding this comment.
I don't think we need to mention export_merge_tree_part_schema_match_mode in the export_merge_tree_part_ignore_extra_source_columns description
|
|
||
| Extra source columns are still read and evaluated (including `MATERIALIZED`/`ALIAS` columns, and any column another kept column's `ALIAS`/`MATERIALIZED` expression depends on) before being dropped, so this setting only changes which columns end up in the destination, not what is computed while reading the part. Type conversion and `export_merge_tree_part_allow_lossy_cast` are applied after columns are matched. | ||
|
|
||
| Error behavior: |
There was a problem hiding this comment.
I think we should remove this error behavior section. It might get outdated quickly.
| } | ||
| } | ||
|
|
||
| TEST(ExportColumnCastsTest, UsesSelectedMatchingMode) |
There was a problem hiding this comment.
It is ok to keep these ut, not asking you to remove them. But in my experience, stateless tests are a much better way of validating & testing features unless it is an inner / deep function that screams for an ut.
Just my 2 cents
| if (allow_lossy_cast) | ||
| return; | ||
|
|
||
| verifyExportColumnCastsAreSafe( |
There was a problem hiding this comment.
This method is questionable. A few lines above, you branch based on the matching policy and loop over the columns. You could just call verifyExportColumnCastIsSafe inline, and then we wouldn't this method.
This method requires you to re-write the column match branching logic, build associative containers and etc. Sounds like an overkill to me.
There was a problem hiding this comment.
A few lines above, you branch based on the matching policy and loop over the columns. You could just call verifyExportColumnCastIsSafe inline, and then we wouldn't this method.
These are separate responsibilities, and it's best not to mix them. We have separate requirements for the partition key, and in my opinion, they're easier to read in the code when they're not mixed with other (matching columns) logic.
| ignore_extra_source_columns); | ||
|
|
||
| if (ignore_extra_source_columns_by_position && source_columns.size() > destination_columns.size()) | ||
| if (!match_by_name && ignore_extra_source_columns && src_has_extra_columns) |
There was a problem hiding this comment.
I think it's a good idea to add a comment explaining why match_by_name does not enter the if block
| match_by_name | ||
| ? ActionsDAG::MatchColumnsMode::Name | ||
| : ActionsDAG::MatchColumnsMode::Position, |
There was a problem hiding this comment.
and this worker only trims/checks extras in positional mode
why tho? I am a bit confused on this one tbh
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>

Since the export partition feature is experimental, I propose abandoning the manifest format with
export_merge_tree_part_schema_mismatch_modeand the corresponding setting.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Renamed the
export_merge_tree_part_schema_mismatch_modesetting for EXPORT PART/EXPORT PARTITION toexport_merge_tree_part_schema_match_mode, with valuesmatch_by_position(default, same behavior as before) andmatch_by_name, which matches destination columns to source columns by name instead of position. Added theexport_merge_tree_part_ignore_extra_source_columnssetting, which allows a source column without a matching destination column to be dropped instead of rejected withNUMBER_OF_COLUMNS_DOESNT_MATCH, in either matching mode.Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: