[
http://issues.apache.org/jira/browse/DERBY-1490?page=comments#action_12445290 ]
A B commented on DERBY-1490:
----------------------------
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(
+ SQLState.LANG_OBJECT_DOES_NOT_EXIST,
+ "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!
> Provide ALTER TABLE RENAME COLUMN functionality
> -----------------------------------------------
>
> Key: DERBY-1490
> URL: http://issues.apache.org/jira/browse/DERBY-1490
> Project: Derby
> Issue Type: New Feature
> Components: Documentation, SQL
> Affects Versions: 10.0.2.0, 10.0.2.1, 10.1.1.0, 10.2.1.6, 10.1.2.1,
> 10.1.3.1
> Reporter: Bryan Pendleton
> Assigned To: Bryan Pendleton
> Attachments: derby1490_v1_needMoreTests.diff,
> renameColumn_v2_with_tests.diff
>
>
> Provide a way to rename a column in an existing table. Possible syntax could
> be:
> ALTER TABLE tablename RENAME COLUMN oldcolumn TO newcolumn;
> Feature should properly handle the possibility that the column is currently
> used in constraints, views, indexes, triggers, etc.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
