Edson Richter
added a comment - 19/Jul/06 19:07 There is additional syntax that could be interesting (from SapDB):
To rename a column:
RENAME tableName.columnName to newColumnName
This syntax is very clear and easy to use (but could cause confusion for people searching for an "ALTER...RENAME" thing.
If Derby team wish to maintain "ALTER TABLE ... ALTER COLUMN", then the sintax could be:
ALTER TABLE tableName ALTER COLUMN columnName RENAME TO newColumnName

Can't both be used?
Sorry if this is a stupid question, but can't DB work with aliases? (so that the easier RENAME directive resolves the more formal ALTER... cmd? This way we can all use what we like best.

Wolf Benz
added a comment - 20/Jul/06 18:36 Can't both be used?
Sorry if this is a stupid question, but can't DB work with aliases? (so that the easier RENAME directive resolves the more formal ALTER... cmd? This way we can all use what we like best.
Philippe

Thanks Edson and Wolf for the suggestions on possible syntax. I see that
Postgres and Oracle appear to implement ALTER TABLE t RENAME COLUMN c TO new_c
while DB2 appears to implement RENAME COLUMN t.c to new_c

I think that in the future it would be possible to support multiple syntax variants. For
now I have concentrated just on implementing the ALTER TABLE syntax as it is
the least amount of work.

Attached is a not-ready-for-commit patch proposal 'derby1490_v1_needMoretests.diff'.
This patch modifies the parser to support the new syntax
ALTER TABLE t RENAME COLUMN c TO new_c
and
ALTER TABLE t RENAME TABLE t TO new_t

The patch also contains a handful of tests, to demonstrate the most rudimentary
aspects of the new command.

I intend to write additional tests later and propose a replacement patch, but I would
like to hear any feedback on this patch, too!

Bryan Pendleton
added a comment - 15/Oct/06 19:51 Thanks Edson and Wolf for the suggestions on possible syntax. I see that
Postgres and Oracle appear to implement ALTER TABLE t RENAME COLUMN c TO new_c
while DB2 appears to implement RENAME COLUMN t.c to new_c
I think that in the future it would be possible to support multiple syntax variants. For
now I have concentrated just on implementing the ALTER TABLE syntax as it is
the least amount of work.
Attached is a not-ready-for-commit patch proposal 'derby1490_v1_needMoretests.diff'.
This patch modifies the parser to support the new syntax
ALTER TABLE t RENAME COLUMN c TO new_c
and
ALTER TABLE t RENAME TABLE t TO new_t
The patch also contains a handful of tests, to demonstrate the most rudimentary
aspects of the new command.
I intend to write additional tests later and propose a replacement patch, but I would
like to hear any feedback on this patch, too!

I had overlooked that Derby already supports a RENAME TABLE and RENAME INDEX statement. Given that,
I think the idea of providing a RENAME COLUMN statement makes a lot of sense.

I shall investigate a possible RENAME COLUMN statement further.

I can see that it might make sense to provide both syntaxes for renaming a column, but I can also see
that one syntax is probably plenty, so if we provide a RENAME COLUMN statement there may be no
point to having an ALTER TABLE t RENAME COLUMN c TO new_c statement.

Also, I'm not sure if there is any purpose in providing support for ALTER TABLE t RENAME TABLE t TO new_t.
That syntax seems rather awkward, and given that there is already a RENAME TABLE statement, I'm
not sure that there is any benefit to providing ALTER TABLE syntax which merely duplicates it.
So I think that part of my first patch makes no sense and should be withdrawn.

I'll be back with a new patch proposal after thinking about this some more...

Bryan Pendleton
added a comment - 18/Oct/06 06:16 I had overlooked that Derby already supports a RENAME TABLE and RENAME INDEX statement. Given that,
I think the idea of providing a RENAME COLUMN statement makes a lot of sense.
I shall investigate a possible RENAME COLUMN statement further.
I can see that it might make sense to provide both syntaxes for renaming a column, but I can also see
that one syntax is probably plenty, so if we provide a RENAME COLUMN statement there may be no
point to having an ALTER TABLE t RENAME COLUMN c TO new_c statement.
Also, I'm not sure if there is any purpose in providing support for ALTER TABLE t RENAME TABLE t TO new_t.
That syntax seems rather awkward, and given that there is already a RENAME TABLE statement, I'm
not sure that there is any benefit to providing ALTER TABLE syntax which merely duplicates it.
So I think that part of my first patch makes no sense and should be withdrawn.
I'll be back with a new patch proposal after thinking about this some more...

Attached is 'renameColumn_v2_with_tests.diff', a patch proposal which is,
I think, ready for review.

I'm interested in all feedback, but I particularly want to draw your attention
to the following questions:

1) The patch proposes to add support for both of these syntaxes:
ALTER TABLE t RENAME COLUMN c1 TO c2
and
RENAME COLUMN t.c1 to c2;

I think it's probably wrong to add both syntaxes, but I'm not sure which
one to prefer, so I've implemented both and am now looking for feedback
about which one is better. (Or, if you think we should implement both, why?)

2) Are there test cases that are missing?

3) Is the handling of active prepared statements satisfactory? With the patch,
if there is a prepared statement against the table, and you rename a
column in the table, the rename is successful, and later attempts to execute
the prepared statement get a "the column does not exist" error. I think it's
pretty reasonable: the error message is accurate and the behavior is
deterministic, but maybe there is room for improvement here?

4) Is the handling of triggers satisfactory? With the patch, you cannot rename
a column which would cause the firing of a trigger, but you are able to
successfully rename a column which is used by the body of the trigger. In
that case, the behavior is similar to that of the prepared statement case
above: the rename column succeeds, and the next time the trigger fires, it
gets a "column does not exist" error. Again, I think the behavior is pretty
reasonable: the error message is clear and the behavior is deterministic,
but again I'm looking for your reaction.

I haven't worked on the documentation yet, partly because until we know which
syntax we prefer, it's hard to know which doc pages to update.

Bryan Pendleton
added a comment - 20/Oct/06 02:14 Attached is 'renameColumn_v2_with_tests.diff', a patch proposal which is,
I think, ready for review.
I'm interested in all feedback, but I particularly want to draw your attention
to the following questions:
1) The patch proposes to add support for both of these syntaxes:
ALTER TABLE t RENAME COLUMN c1 TO c2
and
RENAME COLUMN t.c1 to c2;
I think it's probably wrong to add both syntaxes, but I'm not sure which
one to prefer, so I've implemented both and am now looking for feedback
about which one is better. (Or, if you think we should implement both, why?)
2) Are there test cases that are missing?
3) Is the handling of active prepared statements satisfactory? With the patch,
if there is a prepared statement against the table, and you rename a
column in the table, the rename is successful, and later attempts to execute
the prepared statement get a "the column does not exist" error. I think it's
pretty reasonable: the error message is accurate and the behavior is
deterministic, but maybe there is room for improvement here?
4) Is the handling of triggers satisfactory? With the patch, you cannot rename
a column which would cause the firing of a trigger, but you are able to
successfully rename a column which is used by the body of the trigger. In
that case, the behavior is similar to that of the prepared statement case
above: the rename column succeeds, and the next time the trigger fires, it
gets a "column does not exist" error. Again, I think the behavior is pretty
reasonable: the error message is clear and the behavior is deterministic,
but again I'm looking for your reaction.
I haven't worked on the documentation yet, partly because until we know which
syntax we prefer, it's hard to know which doc pages to update.
Please let me know your comments and suggestions on this patch.

A B
added a comment - 27/Oct/06 20:11 > Does anyone anticipate having time to review this patch proposal?
I'll try to look at this patch this afternoon. Hopefully I can post comments sometime later today or tomorrow...

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?

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!

A B
added a comment - 27/Oct/06 23:59 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!

I tried to applying the patch in my Eclipse env but only the sqlgrammar.jj could be successfully patched. For the remaining files it complained - I will attach a screen shot for those. Wonder if anyone else ran into this.

Anyways, I was able to look at the functionality and things worked fine - could not run the tests though. Regarding the questions you raised:

1) I think RENAME COLUMN t.c1 to c2 seems to be more appropriate. This, as you righly pointed out earlier, seems to be consistent
with RENAME TABLE/INDEX. Of the databases have worked on - I think MySQL and Informix behaves in the same way.

2) I was wondering shouldn't there be some tests using the sqlAuthorization mode too. I manually tried them - a user with select only privileges attempting to rename the column etc. and those seem to work fine.

Rajesh Kartha
added a comment - 28/Oct/06 07:00 I tried to applying the patch in my Eclipse env but only the sqlgrammar.jj could be successfully patched. For the remaining files it complained - I will attach a screen shot for those. Wonder if anyone else ran into this.
Anyways, I was able to look at the functionality and things worked fine - could not run the tests though. Regarding the questions you raised:
1) I think RENAME COLUMN t.c1 to c2 seems to be more appropriate. This, as you righly pointed out earlier, seems to be consistent
with RENAME TABLE/INDEX. Of the databases have worked on - I think MySQL and Informix behaves in the same way.
2) I was wondering shouldn't there be some tests using the sqlAuthorization mode too. I manually tried them - a user with select only privileges attempting to rename the column etc. and those seem to work fine.

I am having trouble applying the patch cleanly with the current trunk (rev# 468656) though. I think its
probably due to some recent check-in with master outputs that conflicts with the current patch. I did
however reviewed the diff files and they look reasonable to me. Some comments regarding your questions:

1) IMHO, since Derby already have support for RENAME TABLE and RENAME INDEX statement,
I think RENAME COLUMN would be a natural fit for this feature than the other form.

2) The patch's test coverage is good and I think adding testcases with synonym would be a nice addition.

3) and 4) I am ok with the behavior you described.

Since the lang/alterTable.sql is already setup to run in SQL authorization mode, I was wondering if it can also be run in legacy mode(non-SQL authorization) as well? (Perhaps another jira entry should address this issue since it applies to other lang tests in general.)

Yip Ng
added a comment - 28/Oct/06 09:03 Hi Bryan, thanks for working on this feature!
I am having trouble applying the patch cleanly with the current trunk (rev# 468656) though. I think its
probably due to some recent check-in with master outputs that conflicts with the current patch. I did
however reviewed the diff files and they look reasonable to me. Some comments regarding your questions:
1) IMHO, since Derby already have support for RENAME TABLE and RENAME INDEX statement,
I think RENAME COLUMN would be a natural fit for this feature than the other form.
2) The patch's test coverage is good and I think adding testcases with synonym would be a nice addition.
3) and 4) I am ok with the behavior you described.
Since the lang/alterTable.sql is already setup to run in SQL authorization mode, I was wondering if it can also be run in legacy mode(non-SQL authorization) as well? (Perhaps another jira entry should address this issue since it applies to other lang tests in general.)

Bryan Pendleton
added a comment - 28/Oct/06 16:03 Thank you Rajesh and Yip and Army. That is very useful feedback.
I think that I shall revise the patch to propose just the RENAME COLUMN statement syntax,
and to take into account the other comments and to update the patch to match current trunk.
I think that it makes sense to file several follow-on issues to track several of the issues
raised by the reviewers. So I'll do that.
Thanks very much !

Thank you very much to all the reviewers; the comments were very helpful!

The major difference between this patch and the previous one is that this patch
includes only the RENAME COLUMN statement. The consensus seemed to be
that there was no need to provide two statements that did the same thing, and
the RENAME COLUMN statement feels more natural.

DERBY-2041, to track the need for updating documentation to describe the new features.

The updated patch also includes several new tests as suggested by the reviewers,
and addresses some whitespace and formatting problems caused by the fact
that my default editor setup uses spaces, not tabs, while the sqlgrammar.jj file
is still largely tab-based, so tab-based patches are preferable.

The patch is updated to a recent trunk, so should apply cleanly.

Reviewers, could you please let me know what you think of this latest patch? Thanks!

Bryan Pendleton
added a comment - 03/Nov/06 23:57 Attached is renameColumn_v3_after_reviews.diff, an updated patch proposal.
Thank you very much to all the reviewers; the comments were very helpful!
The major difference between this patch and the previous one is that this patch
includes only the RENAME COLUMN statement. The consensus seemed to be
that there was no need to provide two statements that did the same thing, and
the RENAME COLUMN statement feels more natural.
I filed two issues for follow-on analysis:
DERBY-2041 , to track the unexpected behavior I saw with triggers,
DERBY-2041 , to track the need for updating documentation to describe the new features.
The updated patch also includes several new tests as suggested by the reviewers,
and addresses some whitespace and formatting problems caused by the fact
that my default editor setup uses spaces, not tabs, while the sqlgrammar.jj file
is still largely tab-based, so tab-based patches are preferable.
The patch is updated to a recent trunk, so should apply cleanly.
Reviewers, could you please let me know what you think of this latest patch? Thanks!

Rajesh Kartha
added a comment - 05/Nov/06 20:23 The latest patch applied successfully.
An initial observations:
Rename column of a non-existent table throws an ' ALTER TABLE" error:
ij> create table btab (i int, j varchar(100));
0 rows inserted/updated/deleted
ij> rename column btab1.i to id;
ERROR 42Y55: 'ALTER TABLE' cannot be performed on 'BTAB1' because it does not exist.
I would expect this to be consistent to following
ij> rename table abcd to bcd;
ERROR 42Y55: 'RENAME TABLE' cannot be performed on 'ABCD' because it does not exist.
Will try out some other scenarios and provide further comments for the new patch.
Meanwhile, I was thinking:
Like 'RENAME COLUMN" isn't 'RENAME CONSTRAINT' a good feature to have in Derby ?
Currently it is a two step process:
ALTER TABLE DROP CONSTRAINT
ALTER TABLE ADD CONSTRAINT
With a 'RENAME CONSTRAINT' this can be done in a single step.
Maybe I will open a JIRA enhancement request to track this.
-Rajesh

Thank you for the updated patch, Bryan. The _v3 patch addresses all of my previous comments and looks great to me.

I do tend to agree with Rajesh about the presence of "ALTER TABLE" in the error messages--that could be slightly confusing since those words do not appear anywhere in the RENAME COLUMN syntax. However, that seems like it could easily come as a follow-up patch to the _v3 one, if you are inclined to address it...

+1 to commit _v3 (though I don't know what came of the sqlAuthorization discussion; was there some resolution as to how (or if) that affects the tests for altertable.sql?)

A B
added a comment - 06/Nov/06 18:21 Thank you for the updated patch, Bryan. The _v3 patch addresses all of my previous comments and looks great to me.
I do tend to agree with Rajesh about the presence of "ALTER TABLE" in the error messages--that could be slightly confusing since those words do not appear anywhere in the RENAME COLUMN syntax. However, that seems like it could easily come as a follow-up patch to the _v3 one, if you are inclined to address it...
+1 to commit _v3 (though I don't know what came of the sqlAuthorization discussion; was there some resolution as to how (or if) that affects the tests for altertable.sql?)

Yip Ng
added a comment - 06/Nov/06 19:04 The _v3 patch applies cleanly. There is one minor problem with the error message when attempting to rename:
(1) A non-existing table (Rajesh has already commented on this one). e.g.:
ij> rename column t2.c1 to w2;
ERROR 42Y55: 'ALTER TABLE' cannot be performed on 'T2' because it does not exist
(2) Renaming a column on a view. e.g.:
ij> create view v1 as select * from t1;
0 rows inserted/updated/deleted
ij> rename column v1.w1 to y1;
ERROR 42Y62: 'ALTER TABLE' is not allowed on 'APP.V1' because it is a view.
Both forms has the string 'ALTER TABLE' which should be 'RENAME COLUMN'. Other than that, The patch looks great! +1 to commit.

Good catch on the error message; it's easy to fix, and I'll revise the patch to say RENAME COLUMN not ALTER TABLE.

Rajesh, I think the suggestion about RENAME CONSTRAINT is intriguing, and I think it is worth filing for future investigation. I don't know how often users with to be able to rename a constraint, but I agree that having to drop and re-add it just in order to rename it is awkward.

Army, regarding the sqlAuthorization issue, I don't have a definite resolution. For now, the tests in lang/altertable.sql run only with sqlAuthorization=true.

Bryan Pendleton
added a comment - 06/Nov/06 19:59 Thanks to all for the review (no, you're not being a pain at all!)
Good catch on the error message; it's easy to fix, and I'll revise the patch to say RENAME COLUMN not ALTER TABLE.
Rajesh, I think the suggestion about RENAME CONSTRAINT is intriguing, and I think it is worth filing for future investigation. I don't know how often users with to be able to rename a constraint, but I agree that having to drop and re-add it just in order to rename it is awkward.
Army, regarding the sqlAuthorization issue, I don't have a definite resolution. For now, the tests in lang/altertable.sql run only with sqlAuthorization=true.

Bryan Pendleton
added a comment - 06/Nov/06 23:14 Attached is renameColumn_v4_fix_error_message.diff, which differs from the previous patch only in that fixes the error messages to say "RENAME COLUMN" rather than "ALTER TABLE".

Attachment renameColumn_v5_with_schema_test.diff differs from the
previous patch proposal only in the addition of a new test that demonstrates
that you can rename a column in a table in the non-current schema by
explicitly specifying a schema name.

That is, the proper syntax for the RENAME COLUMN statement is actually:

Bryan Pendleton
added a comment - 07/Nov/06 23:12 Attachment renameColumn_v5_with_schema_test.diff differs from the
previous patch proposal only in the addition of a new test that demonstrates
that you can rename a column in a table in the non-current schema by
explicitly specifying a schema name.
That is, the proper syntax for the RENAME COLUMN statement is actually:
RENAME COLUMN [schema-name.] table-name.column-name TO new-column-name
Thanks Knut Anders for noticing this; I'll fix the documentation in the DERBY-2042 patch.

Bryan Pendleton
added a comment - 09/Nov/06 00:03 Many thanks to the reviewers for the help and good suggestions! I decided that
the patch seems to be ready and so I've committed it to subversion as revision 472708.