feat: Add handling for updating stored columns on indexes

nielm · nielm · commit eea1b222f153 · 2024-03-11T18:06:22.000+01:00
diff --git a/src/main/java/com/google/cloud/solutions/spannerddl/diff/DdlDiff.java b/src/main/java/com/google/cloud/solutions/spannerddl/diff/DdlDiff.java
@@ -53,6 +53,7 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -126,12 +127,23 @@ public List<String> generateDifferenceStatements(Map<String, Boolean> options)
 
     if (!indexDifferences.entriesDiffering().isEmpty()
         && !options.get(ALLOW_RECREATE_INDEXES_OPT)) {
-      throw new DdlDiffException(
-          "At least one Index differs, and "
-              + ALLOW_RECREATE_INDEXES_OPT
-              + " is not set.\n"
-              + "Indexes: "
-              + Joiner.on(", ").join(indexDifferences.entriesDiffering().keySet()));
+
+      long numChangedIndexes =
+          indexDifferences.entriesDiffering().values().stream()
+              // Check if the indexes only difference is the STORING clause
+              .map((diff) -> DdlDiff.checkIndexDiffOnlyStoring(diff))
+              // Filter out those who have only changed Storing clauses
+              .filter((v) -> !v)
+              .count();
+
+      if (numChangedIndexes > 0) {
+        throw new DdlDiffException(
+            "At least one Index differs, and "
+                + ALLOW_RECREATE_INDEXES_OPT
+                + " is not set.\n"
+                + "Indexes: "
+                + Joiner.on(", ").join(indexDifferences.entriesDiffering().keySet()));
+      }
     }
 
     if (!constraintDifferences.entriesDiffering().isEmpty()
@@ -171,9 +183,13 @@ public List<String> generateDifferenceStatements(Map<String, Boolean> options)
     }
 
     // Drop modified indexes that need to be re-created...
-    for (String indexName : indexDifferences.entriesDiffering().keySet()) {
-      LOG.info("Dropping changed index for re-creation: {}", indexName);
-      output.add("DROP INDEX " + indexName);
+    for (ValueDifference<ASTcreate_index_statement> difference :
+        indexDifferences.entriesDiffering().values()) {
+      if (!checkIndexDiffOnlyStoring(difference)) {
+        LOG.info(
+            "Dropping changed index for re-creation: {}", difference.leftValue().getIndexName());
+        output.add("DROP INDEX " + difference.leftValue().getIndexName());
+      }
     }
 
     // Drop deleted constraints
@@ -256,8 +272,36 @@ public List<String> generateDifferenceStatements(Map<String, Boolean> options)
     // Re-create modified indexes...
     for (ValueDifference<ASTcreate_index_statement> difference :
         indexDifferences.entriesDiffering().values()) {
-      LOG.info("Re-creating changed index: {}", difference.leftValue().getIndexName());
-      output.add(difference.rightValue().toStringOptionalExistClause(false));
+
+      if (checkIndexDiffOnlyStoring(difference)) {
+        LOG.info("Updating STORING clause on index: {}", difference.leftValue().getIndexName());
+        Map<String, String> originalStoredCols =
+            difference.leftValue().getStoredColumnNames().stream()
+                .collect(Collectors.toMap(Function.identity(), Function.identity()));
+        Map<String, String> newStoredCols =
+            difference.rightValue().getStoredColumnNames().stream()
+                .collect(Collectors.toMap(Function.identity(), Function.identity()));
+
+        MapDifference<String, String> colDiff = Maps.difference(originalStoredCols, newStoredCols);
+
+        for (String deletedCol : colDiff.entriesOnlyOnLeft().values()) {
+          output.add(
+              "ALTER INDEX "
+                  + difference.leftValue().getIndexName()
+                  + " DROP STORED COLUMN "
+                  + deletedCol);
+        }
+        for (String deletedCol : colDiff.entriesOnlyOnRight().values()) {
+          output.add(
+              "ALTER INDEX "
+                  + difference.leftValue().getIndexName()
+                  + " ADD STORED COLUMN "
+                  + deletedCol);
+        }
+      } else {
+        LOG.info("Re-creating changed index: {}", difference.leftValue().getIndexName());
+        output.add(difference.rightValue().toStringOptionalExistClause(false));
+      }
     }
 
     // Create new constraints.
@@ -329,6 +373,16 @@ public List<String> generateDifferenceStatements(Map<String, Boolean> options)
     return output.build();
   }
 
+  /** Verify that different indexes are only different in STORING clause. */
+  private static boolean checkIndexDiffOnlyStoring(
+      ValueDifference<ASTcreate_index_statement> indexDifference) {
+
+    return indexDifference
+        .leftValue()
+        .getDefinitionWithoutStoring()
+        .equals(indexDifference.rightValue().getDefinitionWithoutStoring());
+  }
+
   @VisibleForTesting
   static List<String> generateAlterTableStatements(
       ASTcreate_table_statement left, ASTcreate_table_statement right, Map<String, Boolean> options)
diff --git a/src/main/java/com/google/cloud/solutions/spannerddl/parser/ASTcreate_index_statement.java b/src/main/java/com/google/cloud/solutions/spannerddl/parser/ASTcreate_index_statement.java
@@ -21,6 +21,9 @@
 
 import com.google.cloud.solutions.spannerddl.diff.AstTreeUtils;
 import com.google.common.base.Joiner;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
 
 public class ASTcreate_index_statement extends SimpleNode
     implements Comparable<ASTcreate_index_statement> {
@@ -65,6 +68,31 @@ public String toStringOptionalExistClause(boolean includeExists) {
             interleave);
   }
 
+  /**
+   * Gets the Index definition without the Stored column clause (and any EXISTS clause).
+   *
+   * <p>Used for comparing indexes for compatible changes.
+   */
+  public String getDefinitionWithoutStoring() {
+    validateChildren();
+    ASTindex_interleave_clause interleave =
+        getOptionalChildByType(children, ASTindex_interleave_clause.class);
+
+    return Joiner.on(" ")
+        .skipNulls()
+        .join(
+            "CREATE",
+            getOptionalChildByType(children, ASTunique_index.class),
+            getOptionalChildByType(children, ASTnull_filtered.class),
+            "INDEX",
+            getIndexName(),
+            "ON",
+            getChildByType(children, ASTtable.class),
+            getChildByType(children, ASTcolumns.class),
+            (interleave != null ? "," : null),
+            interleave);
+  }
+
   private void validateChildren() {
     for (Node child : children) {
       switch (child.getId()) {
@@ -83,6 +111,14 @@ private void validateChildren() {
     }
   }
 
+  public List<String> getStoredColumnNames() {
+    ASTstored_column_list cols = getOptionalChildByType(children, ASTstored_column_list.class);
+    if (cols == null) {
+      return Collections.emptyList();
+    }
+    return cols.getStoredColumns().stream().map(Object::toString).collect(Collectors.toList());
+  }
+
   @Override
   public int compareTo(ASTcreate_index_statement other) {
     return this.getIndexName().compareTo(other.getIndexName());
diff --git a/src/main/java/com/google/cloud/solutions/spannerddl/parser/ASTstored_column_list.java b/src/main/java/com/google/cloud/solutions/spannerddl/parser/ASTstored_column_list.java
@@ -18,6 +18,7 @@
 
 import com.google.cloud.solutions.spannerddl.diff.AstTreeUtils;
 import com.google.common.base.Joiner;
+import java.util.List;
 
 public class ASTstored_column_list extends SimpleNode {
 
@@ -31,8 +32,10 @@ public ASTstored_column_list(DdlParser p, int id) {
 
   @Override
   public String toString() {
-    return "STORING ( "
-        + Joiner.on(", ").join(AstTreeUtils.getChildrenAssertType(children, ASTstored_column.class))
-        + " )";
+    return "STORING ( " + Joiner.on(", ").join(getStoredColumns()) + " )";
+  }
+
+  public List<ASTstored_column> getStoredColumns() {
+    return AstTreeUtils.getChildrenAssertType(children, ASTstored_column.class);
   }
 }
diff --git a/src/test/java/com/google/cloud/solutions/spannerddl/diff/DdlDiffTest.java b/src/test/java/com/google/cloud/solutions/spannerddl/diff/DdlDiffTest.java
@@ -440,6 +440,53 @@ public void diffCreateIndexDifferentExists() throws DdlDiffException {
         .isEmpty();
   }
 
+  @Test
+  public void diffCreateIndexOnlyStoring() throws DdlDiffException {
+    assertThat(
+            DdlDiff.build(
+                    "CREATE INDEX myindex ON mytable ( col1 ) STORING (col2, col3);",
+                    "CREATE INDEX myindex ON mytable ( col1 ) STORING (col3, col4);")
+                .generateDifferenceStatements(
+                    ImmutableMap.of(
+                        ALLOW_RECREATE_INDEXES_OPT,
+                        false,
+                        ALLOW_DROP_STATEMENTS_OPT,
+                        false,
+                        ALLOW_RECREATE_CONSTRAINTS_OPT,
+                        false)))
+        .containsExactly(
+            "ALTER INDEX myindex DROP STORED COLUMN col2",
+            "ALTER INDEX myindex ADD STORED COLUMN col4");
+  }
+
+  @Test
+  public void diffCreateIndexNotOnlyStoringRecreates() throws DdlDiffException {
+    assertThat(
+            DdlDiff.build(
+                    "CREATE UNIQUE INDEX myindex ON mytable ( col1 ) STORING (col2, col3);",
+                    "CREATE INDEX myindex ON mytable ( col1 ) STORING (col3, col4);")
+                .generateDifferenceStatements(
+                    ImmutableMap.of(
+                        ALLOW_RECREATE_INDEXES_OPT,
+                        true,
+                        ALLOW_DROP_STATEMENTS_OPT,
+                        false,
+                        ALLOW_RECREATE_CONSTRAINTS_OPT,
+                        false)))
+        .containsExactly(
+            "DROP INDEX myindex",
+            "CREATE INDEX myindex ON mytable ( col1 ASC ) STORING ( col3, col4 )");
+  }
+
+  @Test
+  public void diffCreateIndexNotOnlyStoringThrows() throws DdlDiffException {
+    getDiffCheckDdlDiffException(
+        "CREATE UNIQUE INDEX myindex ON mytable ( col1 ) STORING (col2, col3);",
+        "CREATE INDEX myindex ON mytable ( col1 ) STORING (col3, col4);",
+        false,
+        "At least one Index differs, and allowRecreateIndexes is not set");
+  }
+
   private static void getDiffCheckDdlDiffException(
       String originalDdl, String newDdl, boolean allowDropStatements, String exceptionContains) {
     try {
@@ -458,7 +505,9 @@ private static List<String> getDiff(
                 ALLOW_RECREATE_CONSTRAINTS_OPT,
                 true,
                 ALLOW_DROP_STATEMENTS_OPT,
-                allowDropStatements));
+                allowDropStatements,
+                ALLOW_RECREATE_INDEXES_OPT,
+                false));
   }
 
   @Test
diff --git a/src/test/resources/expectedDdlDiff.txt b/src/test/resources/expectedDdlDiff.txt
@@ -250,7 +250,7 @@ ALTER TABLE test1 ADD CONSTRAINT fk_in_table FOREIGN KEY ( col2 ) REFERENCES oth
 == TEST 47 Modifying index with IF NOT EXIST
 
 DROP INDEX test4
-CREATE INDEX test4 ON test1 ( col1 ASC ) STORING ( col2 )
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2 )
 
 == TEST 48 change streams create modify delete in correct order wrt tables
 
@@ -276,7 +276,22 @@ ALTER CHANGE STREAM USER_CHANGES SET OPTIONS (retention_period='7d')
 
 ALTER CHANGE STREAM USER_CHANGES SET OPTIONS (retention_period=NULL)
 
-==
+== TEST 52 Modifying index STORING clause only - add
+
+ALTER INDEX test4 ADD STORED COLUMN col4
+ALTER INDEX test5 ADD STORED COLUMN col2
+
+== TEST 53 Modifying index STORING clause only - remove
 
+ALTER INDEX test4 DROP STORED COLUMN col3
+ALTER INDEX test5 DROP STORED COLUMN col2
+ALTER INDEX test5 DROP STORED COLUMN col3
 
+== TEST 54 Modifying index STORING clause only - add/remove
+
+ALTER INDEX test4 DROP STORED COLUMN col2
+ALTER INDEX test4 ADD STORED COLUMN col4
+ALTER INDEX test4 ADD STORED COLUMN col5
+
+==
 
diff --git a/src/test/resources/newDdl.txt b/src/test/resources/newDdl.txt
@@ -425,7 +425,7 @@ primary key (col1);
 
 == TEST 47 Modifying index with IF NOT EXIST
 
-Create index IF NOT EXISTS test4 on test1 ( col1 ) STORING ( col2 )
+Create UNIQUE NULL_filtered index IF NOT EXISTS test4 on test1 ( col1 ) STORING ( col2 )
 
 == TEST 48 change streams create modify delete in correct order wrt tables
 
@@ -455,4 +455,18 @@ CREATE CHANGE STREAM USER_CHANGES
 CREATE CHANGE STREAM USER_CHANGES
   FOR USER;
 
+== TEST 52 Modifying index STORING clause only - add
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2, col3, col4 );
+CREATE UNIQUE NULL_FILTERED INDEX test5 ON test1 ( col1 ASC ) STORING ( col2 );
+
+== TEST 53 Modifying index STORING clause only - remove
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2 );
+CREATE UNIQUE NULL_FILTERED INDEX test5 ON test1 ( col1 ASC );
+
+== TEST 54 Modifying index STORING clause only - add/remove
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col3, col5, col4 );
+
 ==
diff --git a/src/test/resources/originalDdl.txt b/src/test/resources/originalDdl.txt
@@ -424,7 +424,7 @@ primary key (col1);
 
 == TEST 47 Modifying index with IF NOT EXIST
 
-Create index IF NOT EXISTS test4 on test1 ( col1 )
+Create index IF NOT EXISTS test4 on test1 ( col1 )  STORING ( col2 )
 
 == TEST 48 change streams create modify delete in correct order wrt tables
 
@@ -453,6 +453,20 @@ CREATE CHANGE STREAM USER_CHANGES
 CREATE CHANGE STREAM USER_CHANGES
   FOR USER  OPTIONS (retention_period='7d');
 
+== TEST 52 Modifying index STORING clause only - add
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2, col3 );
+CREATE UNIQUE NULL_FILTERED INDEX test5 ON test1 ( col1 ASC );
+
+== TEST 53 Modifying index STORING clause only - remove
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2, col3 );
+CREATE UNIQUE NULL_FILTERED INDEX test5 ON test1 ( col1 ASC ) STORING ( col2, col3 );
+
+== TEST 54 Modifying index STORING clause only - add/remove
+
+CREATE UNIQUE NULL_FILTERED INDEX test4 ON test1 ( col1 ASC ) STORING ( col2, col3 );
+
 ==
 
 
