Skip to content

Commit be4037a

Browse files
alesapinalesapin
authored
Merge pull request #10895 from ClickHouse/fix_multiple_renames
Fix error in multiple rename commands in a single query.
2 parents c957154 + a504db3 commit be4037a

11 files changed

Lines changed: 143 additions & 42 deletions

‎src/Core/Names.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ using NameSet = std::unordered_set<std::string>;
1515
using NameOrderedSet = std::set<std::string>;
1616
using NameToNameMap = std::unordered_map<std::string, std::string>;
1717
using NameToNameSetMap = std::unordered_map<std::string, NameSet>;
18+
using NameToNameVector = std::vector<std::pair<std::string, std::string>>;
1819

1920
using NameWithAlias = std::pair<std::string, std::string>;
2021
using NamesWithAliases = std::vector<NameWithAlias>;

‎src/Storages/AlterCommands.cpp‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,6 @@ void AlterCommands::validate(const StorageInMemoryMetadata & metadata, const Con
733733
auto all_columns = metadata.columns;
734734
/// Default expression for all added/modified columns
735735
ASTPtr default_expr_list = std::make_shared<ASTExpressionList>();
736-
NameToNameMap renames_map;
737736
for (size_t i = 0; i < size(); ++i)
738737
{
739738
const auto & command = (*this)[i];
@@ -809,31 +808,40 @@ void AlterCommands::validate(const StorageInMemoryMetadata & metadata, const Con
809808
}
810809
else if (command.type == AlterCommand::RENAME_COLUMN)
811810
{
811+
for (size_t j = i + 1; j < size(); ++j)
812+
{
813+
auto next_command = (*this)[j];
814+
if (next_command.type == AlterCommand::RENAME_COLUMN)
815+
{
816+
if (next_command.column_name == command.rename_to)
817+
throw Exception{"Transitive renames in a single ALTER query are not allowed (don't make sense)",
818+
ErrorCodes::NOT_IMPLEMENTED};
819+
else if (next_command.column_name == command.column_name)
820+
throw Exception{"Cannot rename column '" + backQuote(command.column_name)
821+
+ "' to two different names in a single ALTER query",
822+
ErrorCodes::BAD_ARGUMENTS};
823+
}
824+
}
825+
812826
/// TODO Implement nested rename
813-
if (metadata.columns.hasNested(command.column_name))
827+
if (all_columns.hasNested(command.column_name))
814828
{
815829
throw Exception{"Cannot rename whole Nested struct", ErrorCodes::NOT_IMPLEMENTED};
816830
}
817831

818-
if (!metadata.columns.has(command.column_name))
832+
if (!all_columns.has(command.column_name))
819833
{
820834
if (!command.if_exists)
821835
throw Exception{"Wrong column name. Cannot find column " + backQuote(command.column_name) + " to rename",
822836
ErrorCodes::NOT_FOUND_COLUMN_IN_BLOCK};
837+
else
838+
continue;
823839
}
824840

825-
if (metadata.columns.has(command.rename_to))
841+
if (all_columns.has(command.rename_to))
826842
throw Exception{"Cannot rename to " + backQuote(command.rename_to) + ": column with this name already exists",
827843
ErrorCodes::DUPLICATE_COLUMN};
828844

829-
830-
if (renames_map.count(command.column_name))
831-
throw Exception{"Cannot rename column '" + backQuote(command.column_name) + "' to two different names in a single ALTER query", ErrorCodes::BAD_ARGUMENTS};
832-
833-
if (renames_map.count(command.rename_to))
834-
throw Exception{"Rename loop detected in ALTER query",
835-
ErrorCodes::BAD_ARGUMENTS};
836-
837845
String from_nested_table_name = Nested::extractTableName(command.column_name);
838846
String to_nested_table_name = Nested::extractTableName(command.rename_to);
839847
bool from_nested = from_nested_table_name != command.column_name;
@@ -846,7 +854,7 @@ void AlterCommands::validate(const StorageInMemoryMetadata & metadata, const Con
846854
}
847855
else if (!from_nested && !to_nested)
848856
{
849-
renames_map[command.column_name] = command.rename_to;
857+
all_columns.rename(command.column_name, command.rename_to);
850858
}
851859
else
852860
{

‎src/Storages/MergeTree/IMergeTreeDataPart.cpp‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -356,19 +356,26 @@ size_t IMergeTreeDataPart::getFileSizeOrZero(const String & file_name) const
356356
String IMergeTreeDataPart::getColumnNameWithMinumumCompressedSize() const
357357
{
358358
const auto & storage_columns = storage.getColumns().getAllPhysical();
359-
const std::string * minimum_size_column = nullptr;
359+
auto alter_conversions = storage.getAlterConversionsForPart(shared_from_this());
360+
361+
std::optional<std::string> minimum_size_column;
360362
UInt64 minimum_size = std::numeric_limits<UInt64>::max();
361363

362364
for (const auto & column : storage_columns)
363365
{
364-
if (!hasColumnFiles(column.name, *column.type))
366+
auto column_name = column.name;
367+
auto column_type = column.type;
368+
if (alter_conversions.isColumnRenamed(column.name))
369+
column_name = alter_conversions.getColumnOldName(column.name);
370+
371+
if (!hasColumnFiles(column_name, *column_type))
365372
continue;
366373

367-
const auto size = getColumnSize(column.name, *column.type).data_compressed;
374+
const auto size = getColumnSize(column_name, *column_type).data_compressed;
368375
if (size < minimum_size)
369376
{
370377
minimum_size = size;
371-
minimum_size_column = &column.name;
378+
minimum_size_column = column_name;
372379
}
373380
}
374381

‎src/Storages/MergeTree/IMergeTreeDataPart.h‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@ class IMergeTreeDataPart : public std::enable_shared_from_this<IMergeTreeDataPar
128128
String getNewName(const MergeTreePartInfo & new_part_info) const;
129129

130130
/// Returns column position in part structure or std::nullopt if it's missing in part.
131+
///
132+
/// NOTE: Doesn't take column renames into account, if some column renames
133+
/// take place, you must take original name of column for this part from
134+
/// storage and pass it to this method.
131135
std::optional<size_t> getColumnPosition(const String & column_name) const;
132136

133137
/// Returns the name of a column with minimum compressed size (as returned by getColumnSize()).
@@ -291,7 +295,11 @@ class IMergeTreeDataPart : public std::enable_shared_from_this<IMergeTreeDataPar
291295
/// Makes full clone of part in detached/ on another disk
292296
void makeCloneOnDiskDetached(const ReservationPtr & reservation) const;
293297

294-
/// Checks that .bin and .mrk files exist
298+
/// Checks that .bin and .mrk files exist.
299+
///
300+
/// NOTE: Doesn't take column renames into account, if some column renames
301+
/// take place, you must take original name of column for this part from
302+
/// storage and pass it to this method.
295303
virtual bool hasColumnFiles(const String & /* column */, const IDataType & /* type */) const{ return false; }
296304

297305
static UInt64 calculateTotalSizeOnDisk(const DiskPtr & disk_, const String & from);

‎src/Storages/MergeTree/IMergeTreeReader.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -187,17 +187,17 @@ void IMergeTreeReader::evaluateMissingDefaults(Block additional_columns, Columns
187187

188188
NameAndTypePair IMergeTreeReader::getColumnFromPart(const NameAndTypePair & required_column) const
189189
{
190-
auto it = columns_from_part.find(required_column.name);
191-
if (it != columns_from_part.end())
192-
return {it->first, it->second};
193-
194190
if (alter_conversions.isColumnRenamed(required_column.name))
195191
{
196192
String old_name = alter_conversions.getColumnOldName(required_column.name);
197-
it = columns_from_part.find(old_name);
193+
auto it = columns_from_part.find(old_name);
198194
if (it != columns_from_part.end())
199195
return {it->first, it->second};
200196
}
197+
else if (auto it = columns_from_part.find(required_column.name); it != columns_from_part.end())
198+
{
199+
return {it->first, it->second};
200+
}
201201

202202
return required_column;
203203
}

‎src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,23 @@ NameSet injectRequiredColumns(const MergeTreeData & storage, const MergeTreeData
2121
auto all_column_files_missing = true;
2222

2323
const auto & storage_columns = storage.getColumns();
24+
auto alter_conversions = storage.getAlterConversionsForPart(part);
2425
for (size_t i = 0; i < columns.size(); ++i)
2526
{
26-
const auto & column_name = columns[i];
27+
/// possibly renamed
28+
auto column_name_in_part = columns[i];
29+
30+
if (alter_conversions.isColumnRenamed(column_name_in_part))
31+
column_name_in_part = alter_conversions.getColumnOldName(column_name_in_part);
2732

2833
/// column has files and hence does not require evaluation
29-
if (part->hasColumnFiles(column_name, *storage_columns.getPhysical(column_name).type))
34+
if (part->hasColumnFiles(column_name_in_part, *storage_columns.getPhysical(columns[i]).type))
3035
{
3136
all_column_files_missing = false;
3237
continue;
3338
}
3439

35-
const auto column_default = storage_columns.getDefault(column_name);
40+
const auto column_default = storage_columns.getDefault(columns[i]);
3641
if (!column_default)
3742
continue;
3843

‎src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,7 +1085,7 @@ MergeTreeData::MutableDataPartPtr MergeTreeDataMergerMutator::mutatePartToTempor
10851085
auto indices_to_recalc = getIndicesToRecalculate(in, storage_from_source_part, updated_header.getNamesAndTypesList(), context);
10861086

10871087
NameSet files_to_skip = collectFilesToSkip(updated_header, indices_to_recalc, mrk_extension);
1088-
NameToNameMap files_to_rename = collectFilesForRenames(source_part, for_file_renames, mrk_extension);
1088+
NameToNameVector files_to_rename = collectFilesForRenames(source_part, for_file_renames, mrk_extension);
10891089

10901090
if (need_remove_expired_values)
10911091
files_to_skip.insert("ttl.txt");
@@ -1097,7 +1097,8 @@ MergeTreeData::MutableDataPartPtr MergeTreeDataMergerMutator::mutatePartToTempor
10971097
continue;
10981098

10991099
String destination = new_part_tmp_path + "/";
1100-
auto rename_it = files_to_rename.find(it->name());
1100+
String file_name = it->name();
1101+
auto rename_it = std::find_if(files_to_rename.begin(), files_to_rename.end(), [&file_name](const auto & rename_pair) { return rename_pair.first == file_name; });
11011102
if (rename_it != files_to_rename.end())
11021103
{
11031104
if (rename_it->second.empty())
@@ -1328,7 +1329,7 @@ void MergeTreeDataMergerMutator::splitMutationCommands(
13281329
}
13291330

13301331

1331-
NameToNameMap MergeTreeDataMergerMutator::collectFilesForRenames(
1332+
NameToNameVector MergeTreeDataMergerMutator::collectFilesForRenames(
13321333
MergeTreeData::DataPartPtr source_part, const MutationCommands & commands_for_removes, const String & mrk_extension)
13331334
{
13341335
/// Collect counts for shared streams of different columns. As an example, Nested columns have shared stream with array sizes.
@@ -1343,14 +1344,14 @@ NameToNameMap MergeTreeDataMergerMutator::collectFilesForRenames(
13431344
{});
13441345
}
13451346

1346-
NameToNameMap rename_map;
1347+
NameToNameVector rename_vector;
13471348
/// Remove old indices
13481349
for (const auto & command : commands_for_removes)
13491350
{
13501351
if (command.type == MutationCommand::Type::DROP_INDEX)
13511352
{
1352-
rename_map.emplace("skp_idx_" + command.column_name + ".idx", "");
1353-
rename_map.emplace("skp_idx_" + command.column_name + mrk_extension, "");
1353+
rename_vector.emplace_back("skp_idx_" + command.column_name + ".idx", "");
1354+
rename_vector.emplace_back("skp_idx_" + command.column_name + mrk_extension, "");
13541355
}
13551356
else if (command.type == MutationCommand::Type::DROP_COLUMN)
13561357
{
@@ -1360,8 +1361,8 @@ NameToNameMap MergeTreeDataMergerMutator::collectFilesForRenames(
13601361
/// Delete files if they are no longer shared with another column.
13611362
if (--stream_counts[stream_name] == 0)
13621363
{
1363-
rename_map.emplace(stream_name + ".bin", "");
1364-
rename_map.emplace(stream_name + mrk_extension, "");
1364+
rename_vector.emplace_back(stream_name + ".bin", "");
1365+
rename_vector.emplace_back(stream_name + mrk_extension, "");
13651366
}
13661367
};
13671368

@@ -1383,8 +1384,8 @@ NameToNameMap MergeTreeDataMergerMutator::collectFilesForRenames(
13831384

13841385
if (stream_from != stream_to)
13851386
{
1386-
rename_map.emplace(stream_from + ".bin", stream_to + ".bin");
1387-
rename_map.emplace(stream_from + mrk_extension, stream_to + mrk_extension);
1387+
rename_vector.emplace_back(stream_from + ".bin", stream_to + ".bin");
1388+
rename_vector.emplace_back(stream_from + mrk_extension, stream_to + mrk_extension);
13881389
}
13891390
};
13901391
IDataType::SubstreamPath stream_path;
@@ -1394,7 +1395,7 @@ NameToNameMap MergeTreeDataMergerMutator::collectFilesForRenames(
13941395
}
13951396
}
13961397

1397-
return rename_map;
1398+
return rename_vector;
13981399
}
13991400

14001401
NameSet MergeTreeDataMergerMutator::collectFilesToSkip(

‎src/Storages/MergeTree/MergeTreeDataMergerMutator.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,11 @@ class MergeTreeDataMergerMutator
143143
MutationCommands & for_interpreter,
144144
MutationCommands & for_file_renames);
145145

146-
147-
/// Apply commands to source_part i.e. remove some columns in source_part
148-
/// and return set of files, that have to be removed from filesystem and checksums
149-
static NameToNameMap collectFilesForRenames(MergeTreeData::DataPartPtr source_part, const MutationCommands & commands_for_removes, const String & mrk_extension);
146+
/// Apply commands to source_part i.e. remove and rename some columns in
147+
/// source_part and return set of files, that have to be removed or renamed
148+
/// from filesystem and in-memory checksums. Ordered result is important,
149+
/// because we can apply renames that affects each other: x -> z, y -> x.
150+
static NameToNameVector collectFilesForRenames(MergeTreeData::DataPartPtr source_part, const MutationCommands & commands_for_removes, const String & mrk_extension);
150151

151152
/// Files, that we don't need to remove and don't need to hardlink, for example columns.txt and checksums.txt.
152153
/// Because we will generate new versions of them after we perform mutation.

‎tests/queries/0_stateless/01213_alter_rename_column.sql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ SELECT * FROM table_for_rename WHERE key = 1 FORMAT TSVWithNames;
2424

2525
ALTER TABLE table_for_rename RENAME COLUMN value3 to value2; --{serverError 15}
2626
ALTER TABLE table_for_rename RENAME COLUMN value3 TO r1, RENAME COLUMN value3 TO r2; --{serverError 36}
27-
ALTER TABLE table_for_rename RENAME COLUMN value3 TO r1, RENAME COLUMN r1 TO value1; --{serverError 10}
27+
ALTER TABLE table_for_rename RENAME COLUMN value3 TO r1, RENAME COLUMN r1 TO value1; --{serverError 48}
2828

2929
ALTER TABLE table_for_rename RENAME COLUMN value2 TO renamed_value2, RENAME COLUMN value3 TO renamed_value3;
3030

‎tests/queries/0_stateless/01278_alter_rename_combination.reference‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
CREATE TABLE default.rename_table\n(\n `key` Int32, \n `old_value1` Int32, \n `value1` Int32\n)\nENGINE = MergeTree\nORDER BY tuple()\nSETTINGS index_granularity = 8192
2+
key old_value1 value1
3+
1 2 3
4+
CREATE TABLE default.rename_table\n(\n `k` Int32, \n `v1` Int32, \n `v2` Int32\n)\nENGINE = MergeTree\nORDER BY tuple()\nSETTINGS index_granularity = 8192
5+
k v1 v2
6+
1 2 3
7+
4 5 6
8+
---polymorphic---
9+
CREATE TABLE default.rename_table_polymorphic\n(\n `key` Int32, \n `old_value1` Int32, \n `value1` Int32\n)\nENGINE = MergeTree\nORDER BY tuple()\nSETTINGS min_rows_for_wide_part = 10000, index_granularity = 8192
10+
key old_value1 value1
11+
1 2 3
12+
CREATE TABLE default.rename_table_polymorphic\n(\n `k` Int32, \n `v1` Int32, \n `v2` Int32\n)\nENGINE = MergeTree\nORDER BY tuple()\nSETTINGS min_rows_for_wide_part = 10000, index_granularity = 8192
13+
k v1 v2
14+
1 2 3
15+
4 5 6

0 commit comments

Comments
 (0)