-
Notifications
You must be signed in to change notification settings - Fork 7k
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
CLICKHOUSE-4127: Fix ALTER of destination table for the BUFFER engine. #3603
Conversation
630675e
to
453528f
Compare
|
||
ALTER TABLE test.dst DROP COLUMN x, MODIFY COLUMN y String, ADD COLUMN z String DEFAULT 'DEFZ'; | ||
|
||
INSERT INTO test.buffer VALUES (4, 400); |
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.
There was an exception at this line:
<Error> executeQuery: Code: 16, e.displayText() = DB::Exception: There is no column with name x. There are columns: y, z: (when looking at destination table test.dst), e.what() = DB::Exception (from 127.0.0.1:40774) (in query: INSERT INTO test.buffer VALUES), Stack trace:
/home/user/work/ClickHouse/build/dbms/programs/clickhouse-server(DB::Exception::Exception(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int)+0x22) [0x2a67762]
/home/user/work/ClickHouse/build/dbms/programs/clickhouse-server(DB::ITableDeclaration::check(DB::Block const&, bool) const+0x3bc) [0x58918fc]
/home/user/work/ClickHouse/build/dbms/programs/clickhouse-server(DB::BufferBlockOutputStream::write(DB::Block const&)+0x12f) [0x5d00a9f
With my changes this line is successfully executed and causes inserting new row
y | z |
---|---|
400 | DEFZ |
to the table test.dst
.
/// Check table structure. | ||
try | ||
{ | ||
destination->check(block, true); |
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've removed this check to allow writing blocks to StorageBuffer
which doesn't match the structure of the destination table. All necessary data conversion is done by the function StorageBuffer::writeBlockToDestination()
just before writing block to the destination table. This change is necessary in case the structure of the destination table is altered.
dbms/src/Storages/StorageBuffer.cpp
Outdated
} | ||
|
||
if (can_read_from_destination) | ||
streams_from_dst = destination->read(column_names, query_info, context, processed_stage, max_block_size, num_streams); |
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'm not sure about this change. When reading data from a BUFFER table we should process both the BUFFER and the destination table. But after altering the destination table we cannot simply join the data from these two sources because they have different structure.
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.
We can read common subset of columns from the destination table and apply ConvertingBlockInputStream.
I think, it will be more convenient than reading from buffer only.
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.
Using common subset can look strange, I reckon. For example:
CREATE TABLE test.dst (x UInt64, y UInt64) ENGINE = MergeTree ORDER BY tuple();
CREATE TABLE test.buffer (x UInt64, y UInt64) ENGINE = Buffer(test, dst, 1, 99999, 99999, 1, 1, 99999, 99999);
ALTER TABLE test.dst DROP COLUMN x, MODIFY COLUMN y String, ADD COLUMN z String;
SELECT x, y FROM test.buffer; -- will produce only the column y
I mean SELECT col1, col2 FROM table
always produced two-column output for any table. And Clickhouse throws an exception on trying to select non-existing column from a table.
Maybe it would be better to add missing column filled with default values to the blocks retrieved from the destination table. What do you think?
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.
Done.
I've added a new class AddingDefaultBlockInputStream
and created a new source file addMissingDefaults.cpp
to keep common code for AddingDefaultBlockInputStream
and AddingDefaultBlockOutputStream
.
453528f
to
a029034
Compare
e042b28
to
44ca56c
Compare
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en