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

Custom name for index #386

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

alexneisc
Copy link
Contributor

I suggest adding the ability to set custom names for the index.

Postgresql has a length limit on the index name.

These two indexes will have the same name:
create_index :items, [:column1, :column2, :column3, :column4, :column5, :column6, :column7, :column8]
create_index :items, [:column1, :column2, :column3, :column4, :column5, :column6, :column7, :column8, :column9, :column10]

The name will be "items_column1_column2_column3_column4_column5_column6_column7_c". So when creating the second index, an error occurs:
relation "items_column1_column2_column3_column4_column5_column6_column7_c" already exists (Exception)

In this case, we can set a custom name for the second index and avoid the error.
create_index :items, [:column1, :column2, :column3, :column4, :column5, :column6, :column7, :column8], name: :all_columns_index

This can help with the problem from #367

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! This will be super helpful. Just a couple small suggestions.

Also, if you're up for it: what do you think about outputting a warning if the index name is too long and will be truncated by Postgres? Something like "The index name 'the_long_name' will be truncated to 'truncated' by Postgres. Consider adding a custom 'name' for this index" This would be in a separate PR though so we can get this in quick :D

And no worries if you don't have time! I can open an issue about it to tackle it later

Comment on lines 25 to 33
def initialize(@table : Symbol, @columns : Columns | Nil = nil, @if_exists = false, @on_delete = :do_nothing, @name : String? | Symbol? = nil)
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def initialize(@table : Symbol, @columns : Columns | Nil = nil, @if_exists = false, @on_delete = :do_nothing, @name : String? | Symbol? = nil)
end
@columns : Columns?
@name : Symbol | String | Nil
def initialize(@table : Symbol, @columns : Columns, @if_exists = false, @on_delete = :do_nothing)
end
def initialize(@table : Symbol, @name : String | Symbol, @if_exists = false, @on_delete = :do_nothing)
end

This makes it so you can't leave both column and string blank, and makes it clear only one should be used but not both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulcsmith Hm. Good point. Looks great. But there is some ambiguity.
alias Columns = Symbol | Array(Symbol)

If I send the parameter as a String, I get @name. But if I send the parameter as a Symbol, I get @columns. I think this can be confusing.

I'm not sure how to do the right thing. What do you think?

Comment on lines +62 to +68
else
raise "No name or columns specified for drop_index"
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can remove this if the the initializers ensure that one or the other is set since this will never be hit

@alexneisc
Copy link
Contributor Author

@paulcsmith Thanks for your comment! I am happy to do something useful for Lucky :)

Tomorrow I will deal with the changes. I will definitely deal with the warning about the length of the index. This is an exciting task for me

@paulcsmith
Copy link
Member

@alexneisc Thank you so much! Glad it is exciting work :D

@alexneisc alexneisc force-pushed the custom_name_for_index branch from 85d0bd5 to 2e312ea Compare June 13, 2020 14:32
@alexneisc
Copy link
Contributor Author

@paulcsmith As I mentioned earlier, Columns is of type Symbol | Array (Symbol). So if we want to remove index by its name, we can accidentally delete the index by column. alias Columns = Symbol | Array(Symbol)

So I left the previous code with a little change. Now it has become a little more compact.

Ready for suggestions for improving the code :)

@alexneisc alexneisc force-pushed the custom_name_for_index branch from 2e312ea to e0c60a3 Compare June 13, 2020 14:44
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I can see a few semantic changes we could make later, but as for functionality, this is great. I'm gonna 👍 and we can worry about clean up stuff in some later PRs.

@paulcsmith
Copy link
Member

Agreed. We can do some method signature cleanup later since it is more complex than I thought! Thanks for the clear explanation and the nice work on this @alexneisc

@paulcsmith paulcsmith merged commit 1ce294a into luckyframework:master Jun 19, 2020
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.

3 participants