Merge lp:~mmcm/akiban-server/alter-table-add-identity into lp:~akiban-technologies/akiban-server/trunk

[Mike McMahon]

Track added sequence when ALTER TABLE ADD COLUMN adds an identity column.

AlterTableDDLTest#addColumnSerialPk already tests that DDLFunctions is called with the right information, but it uses a mock that only records what was requested of DXL.

The oversight was in the actual AIS implementation. So, add a new test down there this time.

[Thomas Jones-Low (tjoneslo)]

Fixes the problem as stated.

review: Approve

=== modified file 'src/main/java/com/akiban/ais/util/TableChangeValidator.java'
--- src/main/java/com/akiban/ais/util/TableChangeValidator.java	2013-05-28 21:18:45 +0000
+++ src/main/java/com/akiban/ais/util/TableChangeValidator.java	2013-06-05 01:18:27 +0000
@@ -420,6 +420,13 @@
                         droppedSequences.add(oldColumn.getIdentityGenerator().getSequenceName());
                     }
                 } break;
+                case ADD: {
+                    Column newColumn = newTable.getColumn(change.getNewName());
+                    Sequence newSeq = newColumn.getIdentityGenerator();
+                    if(newSeq != null) {
+                        addedIdentity.add(newColumn.getName());
+                    }
+                } break;
             }
         }
 

=== modified file 'src/test/java/com/akiban/ais/util/TableChangeValidatorTest.java'
--- src/test/java/com/akiban/ais/util/TableChangeValidatorTest.java	2013-03-22 20:05:57 +0000
+++ src/test/java/com/akiban/ais/util/TableChangeValidatorTest.java	2013-06-05 01:18:27 +0000
@@ -49,6 +49,7 @@
     private static final String TABLE = "t";
     private static final TableName TABLE_NAME = new TableName(SCHEMA, TABLE);
     private static final String NO_INDEX_CHANGE = "{}";
+    private static final String NO_IDENTITY_CHANGE = "";
 
     private List<TableChange> NO_CHANGES = new ArrayList<>();
 
@@ -78,7 +79,7 @@
                                                  ChangeLevel expectedChangeLevel) {
         return validate(t1, t2, columnChanges, indexChanges, expectedChangeLevel,
                         asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE, "PRIMARY", "PRIMARY")),
-                        false, false, NO_INDEX_CHANGE, false);
+                        false, false, NO_INDEX_CHANGE, false, NO_IDENTITY_CHANGE);
     }
 
     private static TableChangeValidator validate(UserTable t1, UserTable t2,
@@ -87,7 +88,7 @@
                                                  List<String> expectedChangedTables) {
         return validate(t1, t2, columnChanges, indexChanges, expectedChangeLevel,
                         expectedChangedTables,
-                        false, false, NO_INDEX_CHANGE, false);
+                        false, false, NO_INDEX_CHANGE, false, NO_IDENTITY_CHANGE);
     }
 
     private static TableChangeValidator validate(UserTable t1, UserTable t2,
@@ -97,7 +98,8 @@
                                                  boolean expectedParentChange,
                                                  boolean expectedPrimaryKeyChange,
                                                  String expectedAutoGroupIndexChange,
-                                                 boolean autoIndexChanges) {
+                                                 boolean autoIndexChanges,
+                                                 String expectedIdentityChange) {
         TableChangeValidator validator = new TableChangeValidator(t1, t2, columnChanges, indexChanges, autoIndexChanges);
         validator.compareAndThrowIfNecessary();
         assertEquals("Final change level", expectedChangeLevel, validator.getFinalChangeLevel());
@@ -106,6 +108,7 @@
         assertEquals("Changed tables", expectedChangedTables.toString(), validator.getAllChangedTables().toString());
         assertEquals("Affected group index", expectedAutoGroupIndexChange, validator.getAffectedGroupIndexes().toString());
         assertEquals("Unmodified changes", "[]", validator.getUnmodifiedChanges().toString());
+        assertEquals("Changed identity", expectedIdentityChange, identityChangeDesc(validator.getAllChangedTables()));
         return validator;
     }
 
@@ -122,7 +125,19 @@
         return ChangedTableDescription.toString(oldName, newName, newGroup, parentChange, map(indexPairs));
     }
 
-
+    private static String identityChangeDesc(Collection<ChangedTableDescription> tableChanges) {
+        StringBuilder str = new StringBuilder();
+        for (ChangedTableDescription change : tableChanges) {
+            if (!change.getDroppedSequences().isEmpty()) {
+                str.append("-").append(change.getDroppedSequences());
+            }
+            if (!change.getIdentityAdded().isEmpty()) {
+                str.append("+").append(change.getIdentityAdded());
+            }
+        }
+        return str.toString();
+    }
+    
     //
     // Table
     //
@@ -179,6 +194,18 @@
     }
 
     @Test
+    public void addIdentityColumn() {
+        final TableName SEQ_NAME = new TableName(SCHEMA, "seq-1");
+        UserTable t1 = table(builder(TABLE_NAME).colBigInt("x"));
+        UserTable t2 = table(builder(TABLE_NAME).colBigInt("x").colBigInt("id").pk("id").sequence(SEQ_NAME.getTableName()));
+        t2.getColumn("id").setIdentityGenerator(t2.getAIS().getSequence(SEQ_NAME));
+        t2.getColumn("id").setDefaultIdentity(true);
+        validate(t1, t2, asList(TableChange.createAdd("id")), asList(TableChange.createAdd("PRIMARY")), ChangeLevel.GROUP,
+                 asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE)),
+                 false, true, NO_INDEX_CHANGE, false, "+[id]");
+    }
+
+    @Test
     public void dropColumn() {
         UserTable t1 = table(builder(TABLE_NAME).colBigInt("id").colBigInt("x").pk("id"));
         UserTable t2 = table(builder(TABLE_NAME).colBigInt("id").pk("id"));
@@ -220,7 +247,10 @@
         UserTable t2 = table(builder(TABLE_NAME).colLong("id", false).pk("id").sequence(SEQ_NAME.getTableName()));
         t2.getColumn("id").setIdentityGenerator(t2.getAIS().getSequence(SEQ_NAME));
         t2.getColumn("id").setDefaultIdentity(true);
-        validate(t1, t2, asList(TableChange.createModify("id", "id")), NO_CHANGES, ChangeLevel.METADATA);
+        validate(t1, t2, asList(TableChange.createModify("id", "id")), NO_CHANGES, ChangeLevel.METADATA,
+                 asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE, "PRIMARY", "PRIMARY")),
+                 false, false, NO_INDEX_CHANGE, false,
+                 "+[id]");
     }
 
     @Test
@@ -230,7 +260,10 @@
         t1.getColumn("id").setIdentityGenerator(t1.getAIS().getSequence(SEQ_NAME));
         t1.getColumn("id").setDefaultIdentity(true);
         UserTable t2 = table(builder(TABLE_NAME).colLong("id", false).pk("id"));
-        validate(t1, t2, asList(TableChange.createModify("id", "id")), NO_CHANGES, ChangeLevel.METADATA);
+        validate(t1, t2, asList(TableChange.createModify("id", "id")), NO_CHANGES, ChangeLevel.METADATA,
+                 asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE, "PRIMARY", "PRIMARY")),
+                 false, false, NO_INDEX_CHANGE, false,
+                 "-[test.seq-1]");
     }
 
     @Test
@@ -408,7 +441,7 @@
                  asList(TableChange.createModify(Index.PRIMARY_KEY_CONSTRAINT, Index.PRIMARY_KEY_CONSTRAINT)),
                  ChangeLevel.GROUP,
                  asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE)),
-                 false, true, NO_INDEX_CHANGE, false);
+                 false, true, NO_INDEX_CHANGE, false, NO_IDENTITY_CHANGE);
     }
 
     @Test
@@ -420,7 +453,7 @@
                  asList(TableChange.createDrop(Index.PRIMARY_KEY_CONSTRAINT)),
                  ChangeLevel.GROUP,
                  asList(changeDesc(TABLE_NAME, TABLE_NAME, false, ParentChange.NONE)),
-                 false, true, NO_INDEX_CHANGE, false);
+                 false, true, NO_INDEX_CHANGE, false, NO_IDENTITY_CHANGE);
     }
 
     @Test
@@ -437,7 +470,7 @@
                  asList(TableChange.createDrop("__akiban_fk")),
                  ChangeLevel.GROUP,
                  asList(changeDesc(TABLE_NAME, TABLE_NAME, true, ParentChange.DROP)),
-                 true, false, NO_INDEX_CHANGE, false);
+                 true, false, NO_INDEX_CHANGE, false, NO_IDENTITY_CHANGE);
     }
 
     @Test
@@ -467,7 +500,8 @@
                 false,
                 true,
                 NO_INDEX_CHANGE,
-                false
+                false,
+                NO_IDENTITY_CHANGE
         );
     }
 
@@ -514,7 +548,8 @@
                 false,
                 false,
                 NO_INDEX_CHANGE,
-                true
+                true, 
+                NO_IDENTITY_CHANGE
         );
     }
 
@@ -544,7 +579,8 @@
                 false,
                 false,
                 "{test.p.x_y=[]}",
-                false
+                false, 
+                NO_IDENTITY_CHANGE
         );
     }
 
@@ -572,7 +608,8 @@
                 true,
                 false,
                 "{test.p.x_y=[], test.p.x_y_z=[]}",
-                false
+                false, 
+                NO_IDENTITY_CHANGE
         );
     }
 }