Skip to content
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 index #126

Merged
merged 7 commits into from
Jun 25, 2021
Merged

Conversation

ytakaya
Copy link

@ytakaya ytakaya commented Jun 21, 2021

This change supports the following features.

  • ADD INDEX
  • DROP INDEX

ddls = append(ddls, fmt.Sprintf("ALTER TABLE %s DROP INDEX %s", g.escapeTableName(desired.table.name), g.escapeSQLName(currentIndex.name)))
ddls = append(ddls, fmt.Sprintf("ALTER TABLE %s ADD %s", g.escapeTableName(desired.table.name), g.generateIndexDefinition(desiredIndex)))
switch g.mode {
case GeneratorModeMssql:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is applicable to all of your patches, but please consider NOT to introduce g.mode to a place where g.mode switch is not used. At least I couldn't understand why you needed to exclude areSameIndexOptions from areSameIndexes, and using a name like generateDDLForCreateIndex only for mssql is pretty confusing while you already have generateIndexDefinition for the same purpose.

Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I see your effort to fix the way you branch your code based on g.mode as explained in the above comment and this comment first? Feel free to modify existing implementations for doing so.

I'm not saying you must not use g.mode switch everywhere. The general idea is that you should use g.mode switches preferably in deeply nested function calls rather than more abstract functions that already do a lot more things (e.g. generateDDLsForCreateTable). For example, if you generate entire DDLs from generateIndexDefinition, you could move a g.mode switch from generateDDLsForCreateTable to generateIndexDefinition.

The reason why I'm suggesting these changes is that first of all, ideally, every g.mode switch should be eliminated. But if you need to introduce it, you should consider introducing it to a trivial location that doesn't really impact an overall code flow or readability. As I said in the above comment, your current patch duplicates a function with similar roles, branches them in a relatively abstract place, and makes it hard to understand what is used by which mode.

Some of the existing codes don't follow the policy and they should be fixed at some point. But I don't want to newly introduce such places.

@k0kubun
Copy link
Collaborator

k0kubun commented Jun 22, 2021

I knew you need g.mode for ALTER TABLE/CREATE INDEX. I'm just talking about where you use it. You should be able to have only one of generateDDLForCreateIndex and generateIndexDefinition and let it switch ALTER TABLE or CREATE INDEX using g.mode in it. (Please read the above comment again for why)

@ytakaya
Copy link
Author

ytakaya commented Jun 22, 2021

I apologize for the lack of explanation of the changes I made.
 The following are the reasons why I needed g.mode.

In mssql, I couldn't use the syntax ALTER TABLE to put an index on an existing table, I had to use the syntax CREATE INDEX. 
My addition of generateDDLForCreateTable is a function for the syntax CREATE TABLE. I made it a separate function because I thought its intent was different from the existing generateIndexDefinition for ALTER TABLE.

https://docs.microsoft.com/en-us/sql/t-sql/statements/alter-table-transact-sql?view=sql-server-2017#syntax-for-disk-based-tables

As for areSameIndexOptions, it was a branch born of my misunderstanding.
 The syntax for the INDEX option existed in the existing parser.y, but it was not used in the generator.
 Therefore, I mistakenly thought that DBs other than mssql were intentionally designed not to compare index options.

As you pointed out, I think that areSameIndexOptions should be removed, and for generateDDLsForCreateTable, putting the branch in an abstract place was not an optimal change.
Thank you for pointing this out, I'll try to fix it to improve the readability of the code.

@k0kubun
Copy link
Collaborator

k0kubun commented Jun 22, 2021

Not sure why you re-posted the comment, but #126 (comment) was the answer to it.

@ytakaya
Copy link
Author

ytakaya commented Jun 23, 2021

I changed the way to make a generateAddIndex and use g.mode in it to branch.

Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks

@k0kubun k0kubun merged commit f4ce03b into sqldef:master Jun 25, 2021
k0kubun added a commit that referenced this pull request Sep 8, 2021
#126 did not take care of IndexOption.Using.
Having both Using and Value seems confusing, so I just removed it.

The `opt.Name == "using"` part is not good, but it's a tradeoff.

Fix #150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants