-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
Partial support for virtual columns #6919
Conversation
…n converting virtual columns on store / read
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.
Thanks for the feedback
PTAL @reltuk |
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.
seems good to me, i might be missing some context on the virtual index support/testing
{ | ||
Name: "merge a virtual column", | ||
SetUpScript: []string{ | ||
"create table t1 (id bigint primary key, v1 bigint, v2 bigint, v3 bigint as (v1 + v2), index (v3))", |
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.
was there a different PR for index support? do we have standalone tests for that?
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.
You're not missing anything, index merges are broken. They're non trivial to fix because we need the engine's help to rebuild them. I just want to get this in before it gets any bigger, going to do another pass to fix indexes / merge and add more tests.
}, | ||
}, | ||
{ | ||
Name: "merge a virtual column created on another branch", |
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.
what happens if you have a schema merge conflict between virtual/generated/real column that are otherwise the identical in type/name/value? do you have to pick one?
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.
Good question for the next PR
@@ -864,6 +864,8 @@ func getTableSchemaAtRef(queryist cli.Queryist, sqlCtx *sql.Context, tableName s | |||
return sch, createStmt, nil | |||
} | |||
|
|||
// schemaFromCreateTableStmt returns a schema for the CREATE TABLE statement given | |||
// TODO: this is substantially incorrect, doesn't handle primary key ordering, probably other things too |
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.
This probably bothers me the most in this PR. Is this func silently breaking things?
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.
Yes, we are for sure printing schema diffs wrong. Will add bats tests on my next pass
Merges don't work yet, coming in a separate PR.
But storage format details should be final.