I reviewed the patch and functionally it looks good to me. I ran the new test cases in altertable.sql and they all passed.
Just some minor things that jumped out at me while I was reviewing:
Two comments on the patch itself:
1. Test cases say "FIXME" for some of the trigger tests. This appears to be correlated with your comment #4 above, except that the Jira comment says that this behavior is "pretty reasonable" while the test itself suggests that there is something here to be fixed. Are we just waiting for community feedback in order to determine which conclusion ("reasonable" vs "FIXME") is the most appropriate?
2. sqlgrammar.jj has the following diff:
+ if (oldColumnReference.getTableNameNode() == null)
+ throw StandardException.newException(
+ "RENAME COLUMN", "(Missing Table)");
From a user perspective this shows up as something like:
ERROR 42Y55: 'RENAME COLUMN' cannot be performed on '(Missing Table)' because it does not exist.
The string literal "(Missing Table)" is hardcoded into the error message because it is passed as an error argument. This means that someone who is using Derby in a different locale will see the phrase "(Missing Table)" in English; it will not be translated. I don't know what the Derby practice regarding message translation is, but my understanding is that it's generally best to restrict hard-coded error tokens to actual SQL syntax words, since syntax words are constant across locales. Thus it's fine to use "RENAME COLUMN" because that's an explicit part of the syntax and it does not change from locale to locale. But "Missing Table" is a locale-specific phrase and so it's best to avoid passing it as an error message argument. If needed, I think a new error message would be a cleaner way to go.
Of course, after writing that I did a quick look in sqlgrammar.jj and I noticed two places where the English word "MISSING" is hard-coded as an error argument. So I guess it has been done before...but it still seems like something to avoid as a general rule.
As an alternative, I wonder if you couldn't just pass the column name as an argument in this case? Ex.
ij> rename column renc_1 to b;
ERROR 42Y55: 'RENAME COLUMN' cannot be performed on 'RENC_1' because it does not exist.
The plus side to this is that the message is locale-independent and there is a clear indication of what part of the command is causing the error. The downside is that this error message does not really express what we want it to express--namely, that the syntax is missing a table name. So again, perhaps the best bet is to create a new error message that explicitly tells what the problem is...?
In response to the questions you asked in your comment above:
#1) Umm...don't know which is better. I think I'll plea "undecided" on this one
#2) I think you've done a great job of covering the various test cases. Thanks for being so thorough! The only test case that I didn't see was one for trying to rename a column to itself, but when I tried that it threw the expected error (i.e. column already exists). I also tried renaming the column on a synonym, which failed as expected, and I verified that renaming a column in a table "renames" it (functionally) in the synonym, as well. So the tests look good to me.
As for #3 and #4, I personally do not have any problems with the behavior as you describe. I do wonder, though, why it is that we allow renaming to occur when it "breaks" a trigger but do not allow it when it would "break" a view or a check constraint. Is this just because, as the altertable.sql test says, the trigger dependency information is not correctly updated? If that's the case, then is there a Jira issue for fixing that particular problem?
Other minor nits that shouldn't block the patch:
- There appears to be a mix of spaces and tabs in the new code, which makes the file a bit harder to read. In some cases the difference is between existing code and new code, which is probably okay. But there are also some places where the new code itself mixes spaces with tabs...
- One line over 80 chars in sqlgrammar.jj...
As I said, nothing major to report here--certainly not anything I would call a "blocker" for the patch. Functionally the patch looks solid to me. Thanks for taking the time to work on this feature!