-
Notifications
You must be signed in to change notification settings - Fork 0
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
Manually recreate a table if any column needs to add/remove constraint #45
Conversation
test/integration/test_server.cpp
Outdated
col2->set_name("name"); | ||
col2->set_type(::fivetran_sdk::DataType::STRING); | ||
col2->set_primary_key(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.
should this be false initially?
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.
no, it was "false" in create-table block above. This is turning an existing column that was previously NOT a primary key into a primary key.
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.
the test block below // Alter Table to make an existing (unique valued) column into a primary key
has the comment // turn existing column into a primary key
auto col2 = request.mutable_table()->add_columns();
col2->set_name("name");
col2->set_type(::fivetran_sdk::DataType::STRING);
col2->set_primary_key(true); // turn existing column into a primary key
so i thought this block should not set this column to a primary key.
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.
huh, you are right; I think I duplicated something somewhere
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 switched it back to false, and refactored to make it a bit easier to read
src/sql_generator.cpp
Outdated
recreate_table = true; | ||
} | ||
} else if (new_col_it->second.type != col.type) { | ||
alter_types.emplace(col.name); |
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.
why can a change of type happen in place?
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.
or rather, why does this check come before the new_col_it->second.primary_key != col.primary_key
check, and take precedence over change of primary key status
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.
if the types are compatible, it will work. If they are not, this should not be happening in the first place, and recreating a table won't really help
D create table a (col short);
D insert into a (col) values (1), (2), (3);
D select * from a;
┌───────┐
│ col │
│ int16 │
├───────┤
│ 1 │
│ 2 │
│ 3 │
└───────┘
D alter table a alter col type INT;
D select * from a;
┌───────┐
│ col │
│ int32 │
├───────┤
│ 1 │
│ 2 │
│ 3 │
└───────┘
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.
oh, that's a good point; I'll switch them
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!
DuckDB does not yet support ALTER TABLE with any constraints modifications (even DROP COLUMN fails if the column has a primary key).
This PR works around the limitation by renaming the original table, creating a new table with the correct structure, and copying all the old data from columns that were present in both the old table and the new table structure into it.
Fixes #44.
Fixes #2 in a better way (by making it work rather than throwing exception).
Fixes ECO-149