Derby
  1. Derby
  2. DERBY-4013

Allow standard SQL syntax: ALTER TABLE ALTER [COLUMN] <col> SET DEFAULT <default>

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • 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, 10.2.2.0, 10.3.1.4, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0
    • Fix Version/s: 10.5.1.1
    • Component/s: SQL
    • Labels:
      None

      Description

      Presently, the Derby syntax is ALTER TABLE ALTER [COLUMN] <col> [WITH] DEFAULT <default>.
      The "SET" keyword is not accepted, only an optional "WITH". It would be good to accept the standard syntax here as well.
      Cf. SQL 2003, section 11.12 <alter column definition> and section 11.13 <set column default clause>.

      Also, DROP DEFAULT is standard syntax not supported, so we should add that, too.

      Repro on trunk:

      $ java org.apache.derby.tools.ij
      ij version 10.5
      ij> connect 'jdbc:derby:wombat;create=true';
      ij> create table t(i int default 0, j int);
      0 rows inserted/updated/deleted
      ij> alter table t alter column j with default 1;
      0 rows inserted/updated/deleted
      ij> insert into t values (default, default);
      1 row inserted/updated/deleted
      ij> select * from t;
      I |J
      -----------------------
      0 |1

      1 row selected
      ij> alter table t alter column j default 2;
      0 rows inserted/updated/deleted
      ij> insert into t values (default, default);
      1 row inserted/updated/deleted
      ij> select * from t;
      I |J
      -----------------------
      0 |1
      0 |2

      2 rows selected
      ij> alter table t alter column j set default 3;
      ERROR 42X01: Syntax error: Encountered "set" at line 1, column 30.

      1. derby-4013d.stat
        0.2 kB
        Dag H. Wanvik
      2. derby-4013d.diff
        8 kB
        Dag H. Wanvik
      3. derby-4013c.stat
        0.2 kB
        Dag H. Wanvik
      4. derby-4013c.diff
        7 kB
        Dag H. Wanvik
      5. derby-4013b.stat
        0.2 kB
        Dag H. Wanvik
      6. derby-4013b.diff
        6 kB
        Dag H. Wanvik
      7. derby-4013.stat
        0.2 kB
        Dag H. Wanvik
      8. derby-4013.diff
        5 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Uploading a small patch for this with new test cases for the syntax variations. I did not find a natural place for them, so I started a new test class, AlterColumnTest.

          Show
          Dag H. Wanvik added a comment - Uploading a small patch for this with new test cases for the syntax variations. I did not find a natural place for them, so I started a new test class, AlterColumnTest.
          Hide
          Knut Anders Hatlen added a comment -

          I haven't tested the patch, only taken a brief look at it, but it looks correct to me. Why do you think it needs a release note?

          Minor nits:

          • javadoc for the test class describes another test ("compressing tables" it says)
          • javadoc to explain tryAndExpect()'s parameters might be helpful
          • tryAndExpect() reinvents JDBC.assertSingleValueResultSet()
          Show
          Knut Anders Hatlen added a comment - I haven't tested the patch, only taken a brief look at it, but it looks correct to me. Why do you think it needs a release note? Minor nits: javadoc for the test class describes another test ("compressing tables" it says) javadoc to explain tryAndExpect()'s parameters might be helpful tryAndExpect() reinvents JDBC.assertSingleValueResultSet()
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this, Knut! Since this isssue is not a bug and too small to merit a feature write-up,
          I figured it would be best to mark it as requiring a note in the release notes (although it is upwards compatible).
          What do you think?

          Ill respin the patch to fix your nits.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this, Knut! Since this isssue is not a bug and too small to merit a feature write-up, I figured it would be best to mark it as requiring a note in the release notes (although it is upwards compatible). What do you think? Ill respin the patch to fix your nits.
          Hide
          Knut Anders Hatlen added a comment -

          Even though it's a small feature, I think it makes more sense to list it under new features than under potential incompatibilities. The Release Note Needed flag is used for issues that require special attention, and adding this issue to that list will take attention away from the issues that really require special attention. Since there's no incompatibility, I'd suggest that you just add it under Features at the 10.5.1 release wiki: http://wiki.apache.org/db-derby/DerbyTenFiveOneRelease (I think that's where the release manager takes the New Features list from when compiling the final release notes, but I may be mistaken.)

          Show
          Knut Anders Hatlen added a comment - Even though it's a small feature, I think it makes more sense to list it under new features than under potential incompatibilities. The Release Note Needed flag is used for issues that require special attention, and adding this issue to that list will take attention away from the issues that really require special attention. Since there's no incompatibility, I'd suggest that you just add it under Features at the 10.5.1 release wiki: http://wiki.apache.org/db-derby/DerbyTenFiveOneRelease (I think that's where the release manager takes the New Features list from when compiling the final release notes, but I may be mistaken.)
          Hide
          Kim Haase added a comment -

          Should we have an associated documentation issue to update the syntax in the ALTER TABLE topic of the Reference Manual? I can file one, or you can.

          Show
          Kim Haase added a comment - Should we have an associated documentation issue to update the syntax in the ALTER TABLE topic of the Reference Manual? I can file one, or you can.
          Hide
          Dag H. Wanvik added a comment -

          Yes, we should definitely change the docs too! Feel free to file an issue for it!

          Show
          Dag H. Wanvik added a comment - Yes, we should definitely change the docs too! Feel free to file an issue for it!
          Hide
          Dag H. Wanvik added a comment -

          Uploading a new version of this patch, derby-4013b, which addresses Knut's nits.
          Regressions were run on the previous version with no errors.

          Show
          Dag H. Wanvik added a comment - Uploading a new version of this patch, derby-4013b, which addresses Knut's nits. Regressions were run on the previous version with no errors.
          Hide
          Dag H. Wanvik added a comment -

          I will remove the release note flag, and add an entry to
          DerbyTenFiveOneRelease, then.

          It seems the semantics of this flag "Release Note Needed" is now
          effectively: "Release note attached (based on
          /tools/release/templates/releaseNote.html), because this issue
          potentially breaks existing applications". In other cases, we should
          either enter an entry to the upcoming release page, and/or rely on the
          release manager to find issues marked "Improvement" or "New features"
          so something can be said about them in the release notes.

          Show
          Dag H. Wanvik added a comment - I will remove the release note flag, and add an entry to DerbyTenFiveOneRelease, then. It seems the semantics of this flag "Release Note Needed" is now effectively: "Release note attached (based on /tools/release/templates/releaseNote.html), because this issue potentially breaks existing applications". In other cases, we should either enter an entry to the upcoming release page, and/or rely on the release manager to find issues marked "Improvement" or "New features" so something can be said about them in the release notes.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag. The 4013b patch looks good to me. I tested a couple of queries in ij, and it seems to work fine. +1 to commit.

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag. The 4013b patch looks good to me. I tested a couple of queries in ij, and it seems to work fine. +1 to commit.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading patch derby-4013c, which also introduced the standard syntax
          for ALTER [COLUMN] <column-name> DROP DEFAULT which is also not supported
          presently (thank to Kim for discovering that) and added tests for
          that.

          Re-running regressions.

          Show
          Dag H. Wanvik added a comment - - edited Uploading patch derby-4013c, which also introduced the standard syntax for ALTER [COLUMN] <column-name> DROP DEFAULT which is also not supported presently (thank to Kim for discovering that) and added tests for that. Re-running regressions.
          Hide
          Knut Anders Hatlen added a comment -

          SQL 2003, 11.14 <drop column default clause> says the following in Syntax Rules:
          > 2) The descriptor of C shall include a default value.

          And in General Rules:
          > 1) The default value is removed from the column descriptor of C.

          Does this imply that if you drop the default value for the same column twice, the second one should fail because C has no default value? And similarly, if you try to drop the default on an identity column, it should fail, whereas the current patch will make it an ordinary (non-identity) column with default NULL?

          Show
          Knut Anders Hatlen added a comment - SQL 2003, 11.14 <drop column default clause> says the following in Syntax Rules: > 2) The descriptor of C shall include a default value. And in General Rules: > 1) The default value is removed from the column descriptor of C. Does this imply that if you drop the default value for the same column twice, the second one should fail because C has no default value? And similarly, if you try to drop the default on an identity column, it should fail, whereas the current patch will make it an ordinary (non-identity) column with default NULL?
          Hide
          Knut Anders Hatlen added a comment -

          I tested DROP DEFAULT on PostgreSQL, and it appears to behave the same way as Derby would with the latest patch. DROP DEFAULT sets the default to NULL, and it can also be used to set the default of an identity column to NULL. Dropping the default twice does not raise an error.

          Show
          Knut Anders Hatlen added a comment - I tested DROP DEFAULT on PostgreSQL, and it appears to behave the same way as Derby would with the latest patch. DROP DEFAULT sets the default to NULL, and it can also be used to set the default of an identity column to NULL. Dropping the default twice does not raise an error.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for checking the standard on the semantics here, Knut. Derby
          does treat no default the same as a default of null, I find.

          If this is strictly according to the standard, I am not sure. At
          least, if there is no defined default, the null value should be used,
          according to section 11.5 default clause, General Rule 3 e):
          "Otherwise, S is set to the null value."

          The way I read it, this is not technically a default, but rather the
          null is used in lieu of a defined default. By that reasoning, the DROP
          default should probably have failed if there is no defined default on
          that column. Up to now, Derby makes no such distinction, since setting
          a default back to "null" is the only way to get rid of another defined
          default [*].

          But I suggest we be lenient here, as is Postgres in your experiment,
          and let DROP DEFAULT be a no-op if there is no default defined for a
          column, otherwise we would have to start distinguishing between an
          explicitly created "null" default (whose dropping would be legal) and
          an implicit null default (by rule 3e above, in which case dropping the
          default would be an error).

          [*]
          This can be seen by first making an explicit non-null default, and
          then changing its value to null:

          ij> alter table t alter i DEFAULT 1;
          ij> select columndefault, columndefaultid from sys.syscolumns where columnname='I';
          COLUMNDEFAULT |COLUMNDEFAULTID
          ----------------------------------------------------
          1 |286cc01e-011e-efd9-cb02-000003ffe058

          ij> alter table t alter i DEFAULT null;
          ij> select columndefault, columndefaultid from sys.syscolumns where columnname='I';
          COLUMNDEFAULT |COLUMNDEFAULTID
          ----------------------------------------------------
          NULL |NULL

          There no longer a UUID identifying to a defined default (i.e. it has been dropped).

          Show
          Dag H. Wanvik added a comment - Thanks for checking the standard on the semantics here, Knut. Derby does treat no default the same as a default of null, I find. If this is strictly according to the standard, I am not sure. At least, if there is no defined default, the null value should be used, according to section 11.5 default clause, General Rule 3 e): "Otherwise, S is set to the null value." The way I read it, this is not technically a default, but rather the null is used in lieu of a defined default. By that reasoning, the DROP default should probably have failed if there is no defined default on that column. Up to now, Derby makes no such distinction, since setting a default back to "null" is the only way to get rid of another defined default [*] . But I suggest we be lenient here, as is Postgres in your experiment, and let DROP DEFAULT be a no-op if there is no default defined for a column, otherwise we would have to start distinguishing between an explicitly created "null" default (whose dropping would be legal) and an implicit null default (by rule 3e above, in which case dropping the default would be an error). [*] This can be seen by first making an explicit non-null default, and then changing its value to null: ij> alter table t alter i DEFAULT 1; ij> select columndefault, columndefaultid from sys.syscolumns where columnname='I'; COLUMNDEFAULT |COLUMNDEFAULTID ---------------------------------------------------- 1 |286cc01e-011e-efd9-cb02-000003ffe058 ij> alter table t alter i DEFAULT null; ij> select columndefault, columndefaultid from sys.syscolumns where columnname='I'; COLUMNDEFAULT |COLUMNDEFAULTID ---------------------------------------------------- NULL |NULL There no longer a UUID identifying to a defined default (i.e. it has been dropped).
          Hide
          Dag H. Wanvik added a comment -

          Regressions ran ok.

          Show
          Dag H. Wanvik added a comment - Regressions ran ok.
          Hide
          Knut Anders Hatlen added a comment -

          I'm fine with the suggested approach. If someone thinks it's important that we distinguish between the no default case and the NULL default case, that could be fixed later, but for now this looks like a fine incremental improvement to me.

          Two tiny nits:

          • there's a pure whitespace diff right above dropTableConstraintDefinition() in sqlgrammar.jj
          • the javadoc comment for wrapAlterColumnDefaultValue() just says "Minion", which is not very descriptive (but it's great that this common code was factored out!)
          Show
          Knut Anders Hatlen added a comment - I'm fine with the suggested approach. If someone thinks it's important that we distinguish between the no default case and the NULL default case, that could be fixed later, but for now this looks like a fine incremental improvement to me. Two tiny nits: there's a pure whitespace diff right above dropTableConstraintDefinition() in sqlgrammar.jj the javadoc comment for wrapAlterColumnDefaultValue() just says "Minion", which is not very descriptive (but it's great that this common code was factored out!)
          Hide
          Kathey Marsden added a comment -

          I see the test was added to the suite:
          + suite.addTest(AlterColumnTest.suite());

          But I don't see AlterColumnTest as part of the patch. Did you maybe not svn add or am I just missing it?

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - I see the test was added to the suite: + suite.addTest(AlterColumnTest.suite()); But I don't see AlterColumnTest as part of the patch. Did you maybe not svn add or am I just missing it? Thanks Kathey
          Hide
          Dag H. Wanvik added a comment -

          Sorry about that, Kathey, the last rev. lacked the new test. Uploading it again.

          Show
          Dag H. Wanvik added a comment - Sorry about that, Kathey, the last rev. lacked the new test. Uploading it again.
          Hide
          Dag H. Wanvik added a comment -

          Uploaded derby-4013d, which fixes Knut's nits.

          Show
          Dag H. Wanvik added a comment - Uploaded derby-4013d, which fixes Knut's nits.
          Hide
          Dag H. Wanvik added a comment -

          Resolving, committed patch derby-4013d as svn 737774.
          If nobody argues that this should be backported, I will close the issue in a bit.

          Show
          Dag H. Wanvik added a comment - Resolving, committed patch derby-4013d as svn 737774. If nobody argues that this should be backported, I will close the issue in a bit.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development