db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "A B (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-1490) Provide ALTER TABLE RENAME COLUMN functionality
Date Fri, 27 Oct 2006 23:00:19 GMT
    [ 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

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

- 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:,,,,,
>            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


View raw message