DAT-18792: review changes - updated attribute name to match exactly t…

…blProperty 'clusteringColumns', extended snapshot check, removed unnecessary user defined property cleanup. Moved sanitizing logic to snapshot generator to avoid spreading of responsibilities.
liquibase · Oct 17, 2024 · 3df12ed · 3df12ed
1 parent 79043b3
commit 3df12ed
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 35 deletions.
diff --git a/...liquibase/ext/databricks/diff/output/changelog/MissingTableChangeGeneratorDatabricks.java b/...liquibase/ext/databricks/diff/output/changelog/MissingTableChangeGeneratorDatabricks.java
@@ -12,8 +12,6 @@
 import liquibase.structure.DatabaseObject;
 import liquibase.structure.core.Table;
 
-import java.util.regex.Pattern;
-
 public class MissingTableChangeGeneratorDatabricks extends MissingTableChangeGenerator {
 
     @Override
@@ -55,7 +53,7 @@ private CreateTableChangeDatabricks getCreateTableChangeDatabricks(ExtendedTable
         createTableChangeDatabricks.setIfNotExists(temp.getIfNotExists());
         createTableChangeDatabricks.setRowDependencies(temp.getRowDependencies());
         if (!clusterColumns.isEmpty()) {
-            createTableChangeDatabricks.setClusterColumns(sanitizeClusterColumns(clusterColumns));
+            createTableChangeDatabricks.setClusterColumns(clusterColumns);
         }
 
         createTableChangeDatabricks.setExtendedTableProperties(extendedTableProperties);
@@ -66,9 +64,4 @@ private CreateTableChangeDatabricks getCreateTableChangeDatabricks(ExtendedTable
     protected CreateTableChange createCreateTableChange() {
         return new CreateTableChangeDatabricks();
     }
-
-    private String sanitizeClusterColumns(String clusterColumnProperty) {
-        Pattern pattern = Pattern.compile("[\\[\\]\\\"]");
-        return clusterColumnProperty.replaceAll(pattern.toString(), "");
-    }
 }
diff --git a/src/main/java/liquibase/ext/databricks/snapshot/jvm/TableSnapshotGeneratorDatabricks.java b/src/main/java/liquibase/ext/databricks/snapshot/jvm/TableSnapshotGeneratorDatabricks.java
@@ -13,13 +13,14 @@
 
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 public class TableSnapshotGeneratorDatabricks extends TableSnapshotGenerator {
 
     private static final String LOCATION = "Location";
     private static final String TBL_PROPERTIES = "tblProperties";
-    private static final String CLUSTER_COLUMNS = "clusterColumns";
+    private static final String CLUSTER_COLUMNS = "clusteringColumns";
     private static final String DETAILED_TABLE_INFORMATION_NODE = "# Detailed Table Information";
 
     @Override
@@ -53,8 +54,8 @@ protected DatabaseObject snapshotObject(DatabaseObject example, DatabaseSnapshot
             }
             Map<String, String> tblProperties = getTblPropertiesMap(database, example.getName());
             if (tblProperties.containsKey(CLUSTER_COLUMNS)) {
-                // used remove, as clusterColumns tblProperty is not allowed in create/alter table statements
-                table.setAttribute(CLUSTER_COLUMNS, tblProperties.remove(CLUSTER_COLUMNS));
+                // removing clusterColumns, as clusterColumns tblProperty is not allowed in create/alter table statements
+                table.setAttribute(CLUSTER_COLUMNS, sanitizeClusterColumns(tblProperties.remove(CLUSTER_COLUMNS)));
             }
             table.setAttribute(TBL_PROPERTIES, getTblPropertiesString(tblProperties));
         }
@@ -78,4 +79,9 @@ private String getTblPropertiesString(Map<String, String> propertiesMap) {
         return csvString.toString().replaceAll(",$", "");
     }
 
+    private String sanitizeClusterColumns(String clusterColumnProperty) {
+        Pattern pattern = Pattern.compile("[\\[\\]\\\"]");
+        return clusterColumnProperty.replaceAll(pattern.toString(), "");
+    }
+
 }
diff --git a/src/main/java/liquibase/ext/databricks/sqlgenerator/CreateTableGeneratorDatabricks.java b/src/main/java/liquibase/ext/databricks/sqlgenerator/CreateTableGeneratorDatabricks.java
@@ -17,9 +17,6 @@
 
 public class CreateTableGeneratorDatabricks extends CreateTableGenerator {
 
-    private static final String CLUSTERING_INFORMATION_TBL_PROPERTY_START = "clusteringColumns=[[";
-
-
     @Override
     public int getPriority() {
         return PRIORITY_DATABASE;
@@ -51,7 +48,7 @@ public Sql[] generateSql(CreateTableStatement statement, Database database, SqlG
             if ((!StringUtils.isEmpty(thisStatement.getTableFormat()))) {
                 finalsql.append(" USING ").append(thisStatement.getTableFormat());
             } else if (thisStatement.getExtendedTableProperties() != null && StringUtils.isNoneEmpty(thisStatement.getExtendedTableProperties().getTblProperties())) {
-                finalsql.append(" TBLPROPERTIES (").append(avoidClusterProperties(thisStatement)).append(")");
+                finalsql.append(" TBLPROPERTIES (").append(thisStatement).append(")");
             } else {
                 finalsql.append(" USING delta TBLPROPERTIES('delta.feature.allowColumnDefaults' = 'supported', 'delta.columnMapping.mode' = 'name', 'delta.enableDeletionVectors' = true)");
             }
@@ -111,23 +108,4 @@ public Sql[] generateSql(CreateTableStatement statement, Database database, SqlG
 
     }
 
-    /**
-     * While we are passing TBLPROPERTIES as raw string into create table statement, especially in cases of
-     * changelog generation we need to sanitize them from 'clusteringColumns' property, otherwise generated changelog
-     * will fail to execute.
-     * Parsing of tblProperties map as an actual Map structured collection should make this approach safer and easier.
-     * @param statement CreateTableStatementDatabricks containing tblProperties raw string
-     * @return tblProperties string without 'clusteringColumns' property if it was present, otherwise untouched
-     * tblProperties raw string.
-     * */
-    private String avoidClusterProperties(CreateTableStatementDatabricks statement) {
-        String tblProperties = statement.getExtendedTableProperties().getTblProperties();
-        if(tblProperties.contains(CLUSTERING_INFORMATION_TBL_PROPERTY_START)) {
-            int clusterColumnsStartIndex = tblProperties.indexOf(CLUSTERING_INFORMATION_TBL_PROPERTY_START);
-            String replaceString = tblProperties.substring(clusterColumnsStartIndex, tblProperties.indexOf("\"]],", clusterColumnsStartIndex) + 4);
-            return tblProperties.replace(replaceString, "");
-        }
-        return tblProperties;
-    }
-
 }
diff --git a/...sources/liquibase/harness/change/expectedSnapshot/databricks/createClusteredTableNew.json b/...sources/liquibase/harness/change/expectedSnapshot/databricks/createClusteredTableNew.json
@@ -4,7 +4,8 @@
       "liquibase.structure.core.Table": [
         {
           "table": {
-            "name": "test_table_clustered_new"
+            "name": "test_table_clustered_new",
+            "clusteringColumns": "test_id,test_present_new"
           }
         }
       ],