- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 170
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
mssqldef: support drop column #123
Conversation
schema/parser.go
Outdated
|
||
name := "DEFAULT" | ||
if opt.Name.String() != "" { | ||
name = opt.Name.String() |
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.
question: which test of mssqldef_test covers this path? I couldn't figure out why this is needed.
When does this become a non-DEFAULT
name, and what are example names?
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 named TestMsqldeffCreateTableDropColumnWithDefault
covers this case.
This is necessary to resolve the constraint name of the default constraint.
A default constraint name like DF__users__name__XXXXXX
will be generated automatically when the default value is set in the sql server as follows
CREATE TABLE users (
id bigint NOT NULL PRIMARY KEY,
name varchar(20) DEFAULT NULL
);
And in order to drop the default constraint, I need to run the following query.
ALTER TABLE users DROP CONSTRAINT DF__users__name__XXXXXX
So I had to parse the default constraint name here.
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 clarification.
So DefaultDefinition#name
is actually a name of a constraint. If this part is about a constraint, I feel the design is again a bit inconsistent with the existing code. For example, when you have UNIQUE
in a column definition, we do NOT put that information as a part of the column, but add a unique index object to the table. This is needed because we need to consider in-column UNIQUE
and a separate UNIQUE KEY index_name(column)
declaration as to the same thing. Do you think a similar design (add a constraint object to the table) makes more sense to the constraints as well? Is there any way in mssql to specify a default constraint?
Another question: Is it possible in mssql to explicitly specify a constraint name? If so, can we add a test case for it? If such a usage doesn't really makes sense (you never want to rename DF__users__name__XXXXX
to something easier to manage/understand), never mind.
One more question: As you can see in psqldef_test, we already have support for CONSTRAINT
. And it's implemented as a ForeignKey
object. Do you think it makes sense for mssql to consider constraints as foreign keys, or are constraints completely a different thing from foreign keys in mssql?
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.
Do you think a similar design (add a constraint object to the table) makes more sense to the constraints as well? Is there any way in mssql to specify a default constraint?
In mssql, I cannot define a default constraint as <table_constraint>
. I need to define default constraint as <column_definition>
. This is what makes it different from the other PRIMARY KEY
, UNIQUE
, FOREIGN KEY
, and CHECK
constraints. So I needed a specific implementation only for the default constraints, which is what I did in this pull request.
The following document is clear about the definition of mssql constraints.
https://docs.microsoft.com/en-us/sql/t-sql/statements/create-table-transact-sql?view=sql-server-2017#syntax-for-memory-optimized-table
Is it possible in mssql to explicitly specify a constraint name?
I can write the following
CREATE TABLE users (
id bigint NOT NULL PRIMARY KEY,
name varchar(20) CONSTRAINT df_name DEFAULT NULL
);
I agree that this way makes the test code clearer.
Do you think it makes sense for mssql to consider constraints as foreign keys, or are constraints completely a different thing from foreign keys in mssql?
Also in mssql, foreign key can be considered as one of the constraints.
I plan to define PRIMARY KEY
and UNIQUE
, FOREIGN KEY
and CHECK
as <table_constraint>
as well as the other DBs.
When dropping a column, it is necessary to find the index of the corresponding column from the index information and remove the constraint in the same way that I remove the constraint of the primary key in this pull request.
https://github.com/k0kubun/sqldef/pull/123/files#diff-d30ef67aecc70d57c316863ba09cbb56a864bc450da7db94a539dadcd81f4ee0R217
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.
In mssql, I cannot define a default constraint as <table_constraint>. I need to define default constraint as <column_definition>
Got it. Then let's have a default constraint as a part of a column definition as you did, unlike other constraints.
I can write the following
I agree that this way makes the test code clearer.
Unless you intend to always use the explicit version, can we actually test both cases? If possible, I want replaceAutoNamedConstraint
to be kept deleted to avoid misleading readers.
I plan to define PRIMARY KEY and UNIQUE, FOREIGN KEY and CHECK as <table_constraint> as well as the other DBs.
👍
schema/ast.go
Outdated
@@ -164,6 +164,11 @@ type Sequence struct { | |||
OwnedBy string | |||
} | |||
|
|||
type DefaultDefinition struct { | |||
name string | |||
value *Value |
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.
Does DEFAULT NULL
define a default constraint? If it's not the case and DEFAULT NULL
is absolutely the same as writing nothing there, I thought we don't need to make this a pointer (nullable) because defaultDef
itself is also a pointer (nullable).
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, NULL defines the default constraint like other value.
schema/ast.go
Outdated
name string | ||
value *Value |
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.
How about calling it constraintName
for the best clarity? Also, I think it's better to clarify it's only for mssql, and should be put lower than value
because value
is the most main field here.
name string | |
value *Value | |
value *Value | |
constraintName string // only for MSSQL |
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 agree your suggestion.
I changed the naming of the sqlparser#ast#DefaultDefinition
in the same way.
schema/generator.go
Outdated
ddl = fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", g.escapeTableName(desiredTable.name), g.escapeSQLName(column.name)) | ||
default: | ||
ddl = fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", g.escapeTableName(desiredTable.name), g.escapeSQLName(column.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.
It seems like L181 and L183 are the same thing and you don't need to branch them.
schema/generator.go
Outdated
for _, index := range currentTable.indexes { | ||
if index.primary { | ||
for _, indexColumn := range index.columns { | ||
if indexColumn.column == columnName { | ||
ddl := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s", g.escapeTableName(currentTable.name), g.escapeSQLName(index.name)) | ||
ddls = append(ddls, ddl) | ||
} | ||
} | ||
} | ||
} |
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.
It's fine to implement L210-215 here because your design of a default constraint is completely MSSQL-specific. However, I think we already implemented DROP of primary keys for other databases prior to column removal, so it seems weird that you need to take care of this here. Could you make either L143-152 or L155-168 take care of this, instead of individually duplicating the implementation only for MSSQL in this function?
Thanks for reviewing, and I modified the code based on the feedback. |
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!
This change supports the DROP COLUMN in sql server.
In mssql, when dropping a column with constraints, I need to remove the constraints first.
I also need to specify the name of the constraint when removing it.
I took the following approach to drop the constrained columns.
So I modified the default attribute of the column struct to preserve the name of the default constraint.
I also added a fix to preserve the constraint name of the primary key.