Bernt M. Johnsen wrote:
Hi all,
Could somebody review this patch (my first)? Now all identifiers should have a max length of 128 (note: MAX_USERID_LENGTH is unchanged).
I reviewed this patch and it looks good to me. I noticed that in the JIRA entry for this bug, you had written that you wanted to do the following:
[ begin quote ]
2) Ensure that all DB2 related constants are prefixed by DB2_
There are now 6 constants which do not have the prefix: MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, MAX_USERID_LENGTH, MAX_DECIMAL_PRECISION_SCALE, DEFAULT_DECIMAL_PRECISION, DEFAULT_DECIMAL_SCALE
These should get the DB2_ prefix if they are DB2-related (are they?)
[ end quote ]
Did you decide to not do this part? For the record, the comments in the code suggest that the first three of these (MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, and MAX_USERID_LENGTH) are definitely DB2-related, so adding the "DB2_" prefix to those would make sense. As for the latter three, one might assume that they are DB2-specific since they were declared in the "DB2Limits.java" file, but maybe that's not a valid assumption...anyone out there know?? On the one hand, I think these "DB2_" prefix changes could easily be checked in later with a different patch, if someone wants to do it. On the other, it might be less inuitive to people browsing changes to see a solo patch that adds a "DB2_" prefix to some constants in "Limits.java"--at the very least, the patch to do so would have to be very well-commented, or else it could lead to confusion.
NOTE: I couldn't get svn diff to reflect that I have renamed one file (done with svn rename). If it is a problem, I can undo the rename and submit a new patch.
I did have problems applying the patch to the my local codeline, and I'm pretty sure it was because of this. In order to get the patch to apply, I manually copied "DB2Limit.java" to "Limits.java"--after doing that (and only that), I was then able to successfully apply the patch. So whoever commits this might need to do the same, or else use some other trick to get the patch to apply correctly.
After applying the patch I ran a couple of the tests that were affected, and they passed as expected.
One last question: did you have a chance to run the "derbyall" suite on this to make sure you caught all of the relevant tests? I'm guessing that you did based on the number of masters that you updated, but it'd be nice to confirm--typically when you submit a patch, it's good to indicate what tests you ran (and on what platforms/JVMs) just so that reviewers know you actually tested your patch before posting it (which you clearly did!) ;)
All in all, looks good to me. Army
