Re: [PATCH](DERBY-783) Enhance ALTER TABLE syntax to allow users to change the next value to be generated for an identity column

[Mamta Satoor]

Hi Satheesh,

 

Thanks for committing the patch.

 

As for your comments,

1)Yes, I will add the RESTART into a non-reserved word list in the parser. Thanks for catching it. I will also add couple test cases which will try to create database objects with name RESTART.

2)I didn't consider changing autoIncrementStart in SYSCOLUMNSRowFactory.java because this value doesn't get looked at when the next value is generated after the RESTART WITH alter table command.

3)RESTART WITH should work fine in soft upgrade mode. I will try soft upgrade test with 10.1 and 10.2 jars for this change.

 

I have attached the patch to JIRA for changes to sqlgrammar.jj and additional test case to cover 1) above. I am running all the tests to make sure nothing breaks. If I see any problems with the patch and the tests, I will report them tomorrow.

 

thanks,

Mamta

 

On 1/20/06, Satheesh Bandaram <[EMAIL PROTECTED]> wrote:

Hi Mamta,

I think we need to mark 'RESTART' as a non-reserved word and also have couple of other comments. This can be submitted separately, so I committed the patch.

Transmitting file data ..............
Committed revision 370885.

Here are some comments:

1. RESTART needs to be marked as non-reserved word. Can you add this to nonReservedKeyword() list? Currently, attempts to create a table with RESTART name fails. (considers it as reserved word)
2. About your change to SYSCOLUMNSRowFactory.java, should we mark autoIncrementStart with the new value?
3. +               {//user asked for restart with a new value, so don't change increment by and original start
   +                       //with values in the SYSCOLUMNS table. Just record the RESTART WITH value as the
   +                       //next value to be generated in the SYSCOLUMNS table
   +                       ColumnDescriptor  column = (ColumnDescriptor)td;
   +                       row.setColumn(SYSCOLUMNS_AUTOINCREMENTVALUE, new SQLLongint(autoincStart));
   +                       row.setColumn(SYSCOLUMNS_AUTOINCREMENTSTART, new SQLLongint(
   +                                       column.getTableDescriptor().getColumnDescriptor(colName).getAutoincStart()));
   +                       row.setColumn(SYSCOLUMNS_AUTOINCREMENTINC, new SQLLongint(
   +                                       column.getTableDescriptor().getColumnDescriptor(colName).getAutoincInc()));
   
4. Is there a need to prevent RESTART option under soft upgrade? I don't think so, since you are not adding any disk footprint, but just thought I will bring it up.
   

Satheesh

Mamta Satoor wrote:

Hi Satheesh,

 

I have rerun the tests and didn't see any failures after doing a sync. Can you please look into commiting the new review package attached to the JIRA entry?

 

thanks,

Mamta

 

On 1/16/06, Mamta Satoor <[EMAIL PROTECTED]> wrote:

Hi Satheesh,

 

Thanks for looking into committing this patch. I have done a sync on my client and will look into resolving the conflicts. After that, I want to rerun all the tests before submitting an updated patch.
 

Mamta
 

On 1/16/06, Satheesh Bandaram <[EMAIL PROTECTED] > wrote:

Hi Mamta,

Since I applied your other patch, this patch can't be applied as is. (since they share some of the same files) Can you update your patch and post it, please?

Satheesh

Mamta Satoor wrote:

Posted this few days back. Anyone has comments?

 

Thanks,

Mamta

 

On 1/3/06, Mamta Satoor <[EMAIL PROTECTED]> wrote:

Hi,

 

I have attached a review package for this feature to the JIRA entry. Following is a brief description of the changes involved.

Changed sqlgrammar.jj to add parser support for ALTER TABLE <tablename> ALTER <columnName> RESTART WITH integer-constant
Also, added another element to the array which keeps track of autoincrement information in the parser. This 4th element will record if the autoincrement column is getting added or it is getting altered for INCREMENT BY value change or it is getting altered for RESTART WITH value change. This information is required later in the compile and execute phase.

In the compile phase, this information is used to see if a user is trying to sneak in a value of 0 for INCREMENT BY. A value of 0 for INCREMENT BY should be caught at the time of autoincrement column add or at the time of autoincrement column alter to change the INCREMENT BY value. At the time of autoincrement column alter to change the RESTART WITH value, the INCREMENT BY value should not be checked. This is done in ColumnDefinitionNode.java. TableElementList generates ColumnInfo which needs to keep track of autoincrement column change status from ColumnDefinitionNode. This infromation in ColumnInfo will be used at execute time.

In the execute phase, we need to know which columns of SYSCOLUMNS table need to be changed for an ALTER TABLE command on the autoincrement column. In the past, we only allowed to change the INCREMENT BY criteria of an autoincrement column but with this feature, it is possible for a user to change the start with value of autoincrement column and leave the INCREMENT BY unchanged. This autoincrement column change information is passed to the execute phase via ColumnInfo.

In order to provide this distinction between ALTER BY..INCREMENT BY.. and ALTER BY..RESTART WITH.., I have had to add a variable in ColumnDefinitionNode.java, ColumnInfo.java and ColumnDescriptor.java. The value of the variable in each of these classes depend on what parser recorded for the autoincrement column status ie adding an autoincrement column/changing INCREMENT BY of the autoincrement column/changing RESTART WITH of the autoincrement column.

Hope this information along with the comments in the code will help in the code review. Please let me know if you have any comments.

The svn stat changes for this feature is as follows
M      java\engine\org\apache\derby\impl\sql\compile\QueryTreeNode.java
M      java\engine\org\apache\derby\impl\sql\compile\ColumnDefinitionNode.java
M      java\engine\org\apache\derby\impl\sql\compile\CreateSchemaNode.java
M      java\engine\org\apache\derby\impl\sql\compile\sqlgrammar.jj
M      java\engine\org\apache\derby\impl\sql\compile\TableElementList.java
M      java\engine\org\apache\derby\impl\sql\compile\CreateViewNode.java
M      java\engine\org\apache\derby\impl\sql\compile\ModifyColumnNode.java
M      java\engine\org\apache\derby\impl\sql\execute\ColumnInfo.java
M      java\engine\org\apache\derby\impl\sql\execute\CreateTableConstantAction.java
M      java\engine\org\apache\derby\impl\sql\execute\AlterTableConstantAction.java
M      java\engine\org\apache\derby\impl\sql\catalog\SYSCOLUMNSRowFactory.java
M      java\engine\org\apache\derby\iapi\sql\dictionary\ColumnDescriptor.java
M      java\testing\org\apache\derbyTesting\functionTests\tests\lang\autoincrement.sql
M      java\testing\org\apache\derbyTesting\functionTests\master\autoincrement.out

I have modified autoincrement.sql to add tests for RESTART WITH. The derbyall suite ran fine on my Windows XP machine with
Sun's jdk142.
 

thanks,

Mamta