Add index length option for MySQL

[abeverley]

This adds an option to be able to specify index lengths for MySQL, such as:

$index->fields( 'id', { name => 'firstname', size => 15 } );

The majority of this PR is updating the other providers to ignore this, and a bunch of tests (including a new basic SQLServer test).

[castaway]

I like the idea, I'd be tempted to try and hide the fact that the fields may be either strings or hashrefs more... that is instead of having ref $_ ? everywhere, make a method field_names on the Index object which just returns the strings, and maybe another one fields_with_lengths, which returns names and/or name(size) strings as appropriate.

That way if we at some point add more attrs or just make them all refs, we only change one place.

[ribasushi]

@castaway thanks for looking - I agree with the please make a helper function part.

@abeverley Recall how I was unsure about the name - after reading various manuals I am now pretty sure size would be a mistake, as it is pretty vague. Would you be opposed to renaming the option to prefix_length: this way it is both much clearer and much more to type, to make a user think (few engines, even within MySQL itself support this, namely only innodb). Additionally there are things like key_block_size which one may want to specify as well some day, and having already a size will be mighty confusing.

I am volunteering to do the renaming work (you've waited long enough on this PR) - I am just making sure you are not opposed to the changes (as you will need to reflect them on your end :/)

[ribasushi]

@ilmari, @mcsnolte: just a heads up that this one needs to go in for next rel, thus delaying things by several days. I was under the impression someone merged this already, my bad entirely :/

[mcsnolte]

@ribasushi got it, thanks for the update!

[abeverley]

On Wed, 2015-11-04 at 06:14 -0800, Peter Rabbitson wrote:

Would you be opposed to renaming the option to prefix_length

Not at all, absolutely fine.

I am volunteering to do the renaming work (you've waited long enough
on this PR)

Fantastic, thanks.

Andy

[abeverley]

On Wed, 2015-11-04 at 03:12 -0800, castaway wrote:

I like the idea, I'd be tempted to try and hide the fact that the
fields may be either strings or hashrefs more... that is instead of
having ref $_ ? everywhere, make a method field_names on the Index
object which just returns the strings, and maybe another one
fields_with_lengths, which returns names and/or name(size) strings as
appropriate.

Thanks @castaway, makes sense. I don't want to hold a release up though,
and I'm out all day today, so would you mind doing that as part of my
Shadowcat ticket please? (assuming you have time today).

Thanks,

Andy

[ribasushi]

@castaway Leaving the part @abeverley requested to you unless I hear otherwise...

[castaway]

@abeverley @ribasushi yes, can do (tho @ribasushi may have to remind me of the best way to add to/update the PR)

[ribasushi]

@castaway Don't worry about proper GitHub etiquette (as if such a thing ever existed). Get his changes in some form, mangle them, then push a branch of your own naming to the main repo. I will pick it up from there.

[castaway]

Fixes pushed to branch on my github clone with same name.. (and @ribasushi poked with it)

[codecov]

Codecov Report

Merging #68 into master will increase coverage by 0.06%.
The diff coverage is 91.3%.

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage    21.5%   21.57%   +0.06%     
==========================================
  Files          71       71              
  Lines       19914    19930      +16     
  Branches     5728     5741      +13     
==========================================
+ Hits         4283     4299      +16     
+ Misses      14922    14921       -1     
- Partials      709      710       +1

Impacted Files	Coverage Δ
lib/SQL/Translator/Schema/Index.pm	91.66% <ø> (ø)	⬆️
lib/SQL/Translator/Producer/HTML.pm	50.58% <0%> (ø)	⬆️
lib/SQL/Translator/Producer/DB2.pm	83% <0%> (-0.84%)	⬇️
lib/SQL/Translator/Producer/SQLite.pm	86.11% <100%> (+0.19%)	⬆️
lib/SQL/Translator/Producer/MySQL.pm	90.52% <100%> (ø)	⬆️
lib/SQL/Translator/Generator/DDL/SQLServer.pm	90.47% <100%> (+3.37%)	⬆️
lib/SQL/Translator/Role/ListAttr.pm	100% <100%> (ø)	⬆️
lib/SQL/Translator/Producer/Oracle.pm	69.75% <100%> (+0.1%)	⬆️
lib/SQL/Translator/Utils.pm	68.65% <100%> (+1.21%)	⬆️
lib/SQL/Translator/Producer/Sybase.pm	68.7% <100%> (-0.53%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86c68a0...0d987f2. Read the comment docs.

[abeverley]

I've just realised that I don't think this ever got merged. I realised by building a new dev machine and finding the use of this feature not working, so would be good to prevent that happening again in the future ;-)

Would it be possible to merge it please? I've merged in @castaway's changes as discussed above and fixed some merge conflicts. Thanks!

[abeverley]

@rabbiveesh - I understand that you have recently taken over this module. Would there be any chance of merging this long-standing PR please? I have updated it to allow merging with the latest version of master. I have to keep patching my installations, so would appreciate it being in the CPAN release! Many thanks.

[abeverley]

@rabbiveesh @nrdvana ping - any chance of merging this please? I've just rebased to master again, which takes a while each time I have to do it, so would be grateful not to have to do it again :-) Thanks.

[nrdvana]

@rabbiveesh As you design that, an interesting unit test for postgres to really test the boundaries would be

create table test (name varchar(50) not null);
create index ix_test2 on test (substr(name, 2, 3) desc, substr(name, 5, 3) asc);

:-)

[rabbiveesh]

@rabbiveesh As you design that, an interesting unit test for postgres to really test the boundaries would be

create table test (name varchar(50) not null);
create index ix_test2 on test (substr(name, 2, 3) desc, substr(name, 5, 3) asc);

:-)

Can you put this on the other ticket?
I think i'm gonna do the PG side of this in a different PR, mainly b/c I haven't settled on how mch work to make the parser do

[nrdvana]

Well, to spell out the thought I had, the column of an index isn't always a column; it could be a function. And there could be two functions of the same column, so the unique check probably needs to be removed. And (I still didn't check this) if existing parsers are returning sql expressions as the 'field' currently, then the thing that the object returns when stringified should probably be an attribute named something like 'original_sql_expr' instead of 'name'. Quoting the name in a producer should probably only happen if it is known to be a simple name. And all these things probably need to be thought about now in order to make the postgres parser/producer reasonable to implement.

[rabbiveesh]

I think that the design we settled on is sufficiently encapsulated that we can make changes later.
My first thought is that to handle where the field is really an expression, we should have an expression flag in the extra hash, which the postgres producer will then use to skip quoting even when it's on. Hopefully it won't be too hard to recognize an expression, I haven't worked on the parsers in a while.

re uniq - in my implemenation, i actually removed that, b/c the fields of an Index need to be coerced into the new IndexField objects, so that's handled there. Though i see the logic in not allowing duplicates (allows calling code to be lazy in programatically adding column names or whatever).

[rabbiveesh]

The truth is, using expressions as index fields would actually need it's own support completely, b/c it would break the very naive is_valid check on ::Index.

We should write up all these notes in a new ticket (hint hint)

[rabbiveesh]

Fine, i opened the other ticket

[rabbiveesh]

@abeverley I have opened the PR abeverley#1 with my changes. Please review it w/ your thoughts and/or add to its completeness.

If you could add the extra key to the YAML and JSON parser etc in the event that it has data, that would be awesome

[abeverley]

If you could add the extra key to the YAML and JSON parser etc in the event that it has data, that would be awesome

Ah, I just saw this comment after posting my other one. I will merge your changes and then add the key as suggested.

[abeverley]

Okay, I've just done that (bfc7565). I've only done it in the YAML producer, as the JSON one seems to be structured differently and doesn't return a list of fields. I noticed that I didn't touch the JSON producer originally, possibly because of that limitation. The YAML producer was changed in this commit (4a06a63) - I am not sure if that should also be applied to the JSON producer?

[rabbiveesh]

Super sorry that I've dropped the ball on this one.

Where are we holding?

[abeverley]

Super sorry that I've dropped the ball on this one.

Where are we holding?

No problem at all. Personally I think it is all good to go. I've used your revised implementation (much better), made some minor updates and fixed the tests. The only thing I haven't done is update the JSON producer, but that's because it seems to not be applicable so there is nothing to update.

Let me know if you disagree :)

[rabbiveesh]

i'm gonna do the last touches here

[rabbiveesh]

@abeverley made a PR against your PR again, merge it up + we're good to go over here

[rabbiveesh]

Awesome! So i'll merge this up, and do a dev release, I hope to get around to implementing the parser side of this feature and then we can do a real release

[abeverley]

Brilliant, thanks @rabbiveesh - really pleased we've got this merged now. It was definitely worth spending a bit of time on it. As a final test I've just tried it on my local code base and I can confirm it works as expected.