|
[
Permalink
| « Hide
]
Dag H. Wanvik added a comment - 09/Jan/09 08:11 PM
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.
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() 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. 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.)
Yes, we should definitely change the docs too! Feel free to file an issue for it!
Uploading a new version of this patch, derby-4013b, which addresses Knut's nits.
Regressions were run on the previous version with no errors. 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. 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.
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. 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? 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.
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). 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!) 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 Sorry about that, Kathey, the last rev. lacked the new test. Uploading it again.
Uploaded derby-4013d, which fixes Knut's nits.
Resolving, committed patch derby-4013d as svn 737774.
If nobody argues that this should be backported, I will close the issue in a bit. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Resolved
Minor
Allow standard SQL syntax: ALTER TABLE ALTER [COLUMN] <col> SET DEFAULT <default>
