Derby
  1. Derby
  2. DERBY-1490

Provide ALTER TABLE RENAME COLUMN functionality

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.2.0, 10.0.2.1, 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6
    • Fix Version/s: 10.3.1.4
    • Component/s: Documentation, SQL
    • Labels:
      None

      Description

      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.

      1. renameColumn_v5_with_schema_test.diff
        24 kB
        Bryan Pendleton
      2. renameColumn_v4_fix_error_message.diff
        22 kB
        Bryan Pendleton
      3. renameColumn_v3_after_review.diff
        22 kB
        Bryan Pendleton
      4. renameColumn_v2_with_tests.diff
        38 kB
        Bryan Pendleton
      5. derby1490_v1_needMoreTests.diff
        6 kB
        Bryan Pendleton
      6. 1490_cannot_patch.jpg
        65 kB
        Rajesh Kartha

        Issue Links

          Activity

          Hide
          Edson Richter added a comment -

          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

          Show
          Edson Richter added a comment - 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
          Hide
          Wolf Benz added a comment -

          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

          Show
          Wolf Benz added a comment - 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
          Hide
          Bryan Pendleton added a comment -

          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!

          Show
          Bryan Pendleton added a comment - 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!
          Hide
          Bryan Pendleton added a comment -

          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...

          Show
          Bryan Pendleton added a comment - 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...
          Hide
          Bryan Pendleton added a comment -

          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.

          Show
          Bryan Pendleton added a comment - 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.
          Hide
          Bryan Pendleton added a comment -

          Does anyone anticipate having time to review this patch proposal?

          Show
          Bryan Pendleton added a comment - Does anyone anticipate having time to review this patch proposal?
          Hide
          A B added a comment -

          > 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...

          Show
          A B added a comment - > 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...
          Hide
          Yip Ng added a comment -

          Hi Bryan, I have some time during the weekend to review this patch.

          Show
          Yip Ng added a comment - Hi Bryan, I have some time during the weekend to review this patch.
          Hide
          A B added a comment -

          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!

          Show
          A B added a comment - 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!
          Hide
          Rajesh Kartha added a comment -

          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.

          Show
          Rajesh Kartha added a comment - 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.
          Hide
          Rajesh Kartha added a comment -

          Patch error in Eclipse

          Show
          Rajesh Kartha added a comment - Patch error in Eclipse
          Hide
          Yip Ng added a comment -

          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.)

          Show
          Yip Ng added a comment - 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.)
          Hide
          Bryan Pendleton added a comment -

          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 !

          Show
          Bryan Pendleton added a comment - 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 !
          Hide
          Bryan Pendleton added a comment -

          Clear patch available while I work on revising the patch.

          Show
          Bryan Pendleton added a comment - Clear patch available while I work on revising the patch.
          Hide
          Bryan Pendleton added a comment -

          If/when DERBY-2041 is addressed, this test script will need to change.

          Show
          Bryan Pendleton added a comment - If/when DERBY-2041 is addressed, this test script will need to change.
          Hide
          Bryan Pendleton added a comment -

          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!

          Show
          Bryan Pendleton added a comment - 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!
          Hide
          Rajesh Kartha added a comment -

          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

          Show
          Rajesh Kartha added a comment - 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
          Hide
          A B added a comment -

          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?)

          Show
          A B added a comment - 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?)
          Hide
          Yip Ng added a comment -

          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.

          Show
          Yip Ng added a comment - 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.
          Hide
          Rajesh Kartha added a comment -

          Sorry to be a pain , here is another reference to 'ALTER TABLE' .

          ij> rename column sys.systables.tablenam to tabnameraj;
          ERROR 42X62: 'ALTER TABLE' is not allowed in the 'SYS' schema.

          Otherwise, I think the patch looks good.

          Thanks Bryan.

          -Rajesh

          Show
          Rajesh Kartha added a comment - Sorry to be a pain , here is another reference to 'ALTER TABLE' . ij> rename column sys.systables.tablenam to tabnameraj; ERROR 42X62: 'ALTER TABLE' is not allowed in the 'SYS' schema. Otherwise, I think the patch looks good. Thanks Bryan. -Rajesh
          Hide
          Bryan Pendleton added a comment -

          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.

          Show
          Bryan Pendleton added a comment - 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.
          Hide
          Bryan Pendleton added a comment -

          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".

          Show
          Bryan Pendleton added a comment - 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".
          Hide
          Bryan Pendleton added a comment -

          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.

          Show
          Bryan Pendleton added a comment - 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.
          Hide
          Bryan Pendleton added a comment -

          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.

          Show
          Bryan Pendleton added a comment - 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.
          Hide
          Andrew McIntyre added a comment -

          This issue has been resolved for over a year with no further movement. Closing.

          Show
          Andrew McIntyre added a comment - This issue has been resolved for over a year with no further movement. Closing.

            People

            • Assignee:
              Bryan Pendleton
              Reporter:
              Bryan Pendleton
            • Votes:
              1 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development