fix: Remove StorageType from ClpTableHandle since it will now be a cluster-level property. (#18)

wraymo · web-flow · commit e3bc083c4935 · 2025-06-24T11:35:29.000-04:00
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpTableHandle.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpTableHandle.java
@@ -27,17 +27,12 @@ public class ClpTableHandle
 {
     private final SchemaTableName schemaTableName;
     private final String tablePath;
-    private final StorageType storageType;
 
     @JsonCreator
-    public ClpTableHandle(
-            @JsonProperty("schemaTableName") SchemaTableName schemaTableName,
-            @JsonProperty("tablePath") String tablePath,
-            @JsonProperty("storageType") StorageType storageType)
+    public ClpTableHandle(@JsonProperty("schemaTableName") SchemaTableName schemaTableName, @JsonProperty("tablePath") String tablePath)
     {
         this.schemaTableName = schemaTableName;
         this.tablePath = tablePath;
-        this.storageType = storageType;
     }
 
     @JsonProperty
@@ -52,16 +47,10 @@ public String getTablePath()
         return tablePath;
     }
 
-    @JsonProperty
-    public StorageType getStorageType()
-    {
-        return storageType;
-    }
-
     @Override
     public int hashCode()
     {
-        return Objects.hash(schemaTableName, tablePath, storageType);
+        return Objects.hash(schemaTableName, tablePath);
     }
 
     @Override
@@ -74,9 +63,7 @@ public boolean equals(Object obj)
             return false;
         }
         ClpTableHandle other = (ClpTableHandle) obj;
-        return this.schemaTableName.equals(other.schemaTableName) &&
-                this.tablePath.equals(other.tablePath) &&
-                this.storageType == other.storageType;
+        return this.schemaTableName.equals(other.schemaTableName) && this.tablePath.equals(other.tablePath);
     }
 
     @Override
@@ -85,13 +72,6 @@ public String toString()
         return toStringHelper(this)
                 .add("schemaTableName", schemaTableName)
                 .add("tablePath", tablePath)
-                .add("storageType", storageType)
                 .toString();
     }
-
-    public enum StorageType
-    {
-        FS, // Local File System
-        S3
-    }
 }
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMySqlMetadataProvider.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMySqlMetadataProvider.java
@@ -39,7 +39,6 @@ public class ClpMySqlMetadataProvider
     public static final String COLUMN_METADATA_TABLE_COLUMN_NAME = "name";
     public static final String COLUMN_METADATA_TABLE_COLUMN_TYPE = "type";
     public static final String DATASETS_TABLE_COLUMN_NAME = "name";
-    public static final String DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE = "archive_storage_type";
     public static final String DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY = "archive_storage_directory";
 
     // Table suffixes
@@ -49,9 +48,8 @@ public class ClpMySqlMetadataProvider
     // SQL templates
     private static final String SQL_SELECT_COLUMN_METADATA_TEMPLATE = "SELECT * FROM `%s%s" + COLUMN_METADATA_TABLE_SUFFIX + "`";
     private static final String SQL_SELECT_DATASETS_TEMPLATE = format(
-            "SELECT `%s`, `%s`, `%s` FROM `%%s%s`",
+            "SELECT `%s`, `%s` FROM `%%s%s`",
             DATASETS_TABLE_COLUMN_NAME,
-            DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE,
             DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY,
             DATASETS_TABLE_SUFFIX);
 
@@ -102,14 +100,9 @@ public List<ClpTableHandle> listTableHandles(String schemaName)
                 ResultSet resultSet = statement.executeQuery(query)) {
             while (resultSet.next()) {
                 String tableName = resultSet.getString(DATASETS_TABLE_COLUMN_NAME);
-                String archiveStorageType = resultSet.getString(DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE);
                 String archiveStorageDirectory = resultSet.getString(DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY);
                 if (isValidIdentifier(tableName) && archiveStorageDirectory != null && !archiveStorageDirectory.isEmpty()) {
-                    tableHandles.add(
-                            new ClpTableHandle(
-                                    new SchemaTableName(schemaName, tableName),
-                                    archiveStorageDirectory,
-                                    ClpTableHandle.StorageType.valueOf(archiveStorageType.toUpperCase())));
+                    tableHandles.add(new ClpTableHandle(new SchemaTableName(schemaName, tableName), archiveStorageDirectory));
                 }
                 else {
                     log.warn("Ignoring invalid table name found in metadata: %s", tableName);
diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpMetadataDbSetUp.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpMetadataDbSetUp.java
@@ -34,7 +34,6 @@
 import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.COLUMN_METADATA_TABLE_COLUMN_NAME;
 import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.COLUMN_METADATA_TABLE_COLUMN_TYPE;
 import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY;
-import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE;
 import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.DATASETS_TABLE_COLUMN_NAME;
 import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.DATASETS_TABLE_SUFFIX;
 import static com.facebook.presto.plugin.clp.split.ClpMySqlSplitProvider.ARCHIVES_TABLE_COLUMN_ID;
@@ -184,13 +183,9 @@ private static void createDatasetsTable(Statement stmt)
             throws SQLException
     {
         final String createDatasetsTableSql = format(
-                "CREATE TABLE IF NOT EXISTS %s (" +
-                        " %s VARCHAR(255) PRIMARY KEY," +
-                        " %s VARCHAR(64) NOT NULL," +
-                        " %s VARCHAR(4096) NOT NULL)",
+                "CREATE TABLE IF NOT EXISTS %s (%s VARCHAR(255) PRIMARY KEY, %s VARCHAR(4096) NOT NULL)",
                 DATASETS_TABLE_NAME,
                 DATASETS_TABLE_COLUMN_NAME,
-                DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE,
                 DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY);
         stmt.execute(createDatasetsTableSql);
     }
@@ -199,15 +194,13 @@ private static void updateDatasetsTable(Connection conn, String tableName)
             throws SQLException
     {
         final String insertDatasetsTableSql = format(
-                "INSERT INTO %s (%s, %s, %s) VALUES (?, ?, ?)",
+                "INSERT INTO %s (%s, %s) VALUES (?, ?)",
                 DATASETS_TABLE_NAME,
                 DATASETS_TABLE_COLUMN_NAME,
-                DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_TYPE,
                 DATASETS_TABLE_COLUMN_ARCHIVE_STORAGE_DIRECTORY);
         try (PreparedStatement pstmt = conn.prepareStatement(insertDatasetsTableSql)) {
             pstmt.setString(1, tableName);
-            pstmt.setString(2, "fs");
-            pstmt.setString(3, ARCHIVE_STORAGE_DIRECTORY_BASE + tableName);
+            pstmt.setString(2, ARCHIVE_STORAGE_DIRECTORY_BASE + tableName);
             pstmt.executeUpdate();
         }
     }
diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpSplit.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpSplit.java
@@ -31,7 +31,6 @@
 import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.DbHandle;
 import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.getDbHandle;
 import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.setupSplit;
-import static com.facebook.presto.plugin.clp.ClpTableHandle.StorageType.FS;
 import static org.testng.Assert.assertEquals;
 
 @Test(singleThreaded = true)
@@ -77,9 +76,7 @@ public void testListSplits()
             String tablePath = ARCHIVE_STORAGE_DIRECTORY_BASE + tableName;
             List<String> expectedSplits = entry.getValue();
             ClpTableLayoutHandle layoutHandle = new ClpTableLayoutHandle(
-                    new ClpTableHandle(new SchemaTableName(DEFAULT_SCHEMA_NAME, tableName),
-                            tablePath, FS),
-                    Optional.empty());
+                    new ClpTableHandle(new SchemaTableName(DEFAULT_SCHEMA_NAME, tableName), tablePath), Optional.empty());
             List<ClpSplit> splits = clpSplitProvider.listSplits(layoutHandle);
             assertEquals(splits.size(), expectedSplits.size());
 

Original file line number	Diff line number	Diff line change
`@@ -27,17 +27,12 @@ public class ClpTableHandle`
`27`	`27`	`{`
`28`	`28`	`private final SchemaTableName schemaTableName;`
`29`	`29`	`private final String tablePath;`
`30`		`- private final StorageType storageType;`
`31`	`30`
`32`	`31`	`@JsonCreator`
`33`		`- public ClpTableHandle(`
`34`		`- @JsonProperty("schemaTableName") SchemaTableName schemaTableName,`
`35`		`- @JsonProperty("tablePath") String tablePath,`
`36`		`- @JsonProperty("storageType") StorageType storageType)`
	`32`	`+ public ClpTableHandle(@JsonProperty("schemaTableName") SchemaTableName schemaTableName, @JsonProperty("tablePath") String tablePath)`
`37`	`33`	`{`
`38`	`34`	`this.schemaTableName = schemaTableName;`
`39`	`35`	`this.tablePath = tablePath;`
`40`		`- this.storageType = storageType;`
`41`	`36`	`}`
`42`	`37`
`43`	`38`	`@JsonProperty`
`@@ -52,16 +47,10 @@ public String getTablePath()`
`52`	`47`	`return tablePath;`
`53`	`48`	`}`
`54`	`49`
`55`		`- @JsonProperty`
`56`		`- public StorageType getStorageType()`
`57`		`- {`
`58`		`- return storageType;`
`59`		`- }`
`60`		`-`
`61`	`50`	`@Override`
`62`	`51`	`public int hashCode()`
`63`	`52`	`{`
`64`		`- return Objects.hash(schemaTableName, tablePath, storageType);`
	`53`	`+ return Objects.hash(schemaTableName, tablePath);`
`65`	`54`	`}`
`66`	`55`
`67`	`56`	`@Override`
`@@ -74,9 +63,7 @@ public boolean equals(Object obj)`
`74`	`63`	`return false;`
`75`	`64`	`}`
`76`	`65`	`ClpTableHandle other = (ClpTableHandle) obj;`
`77`		`- return this.schemaTableName.equals(other.schemaTableName) &&`
`78`		`- this.tablePath.equals(other.tablePath) &&`
`79`		`- this.storageType == other.storageType;`
	`66`	`+ return this.schemaTableName.equals(other.schemaTableName) && this.tablePath.equals(other.tablePath);`
`80`	`67`	`}`
`81`	`68`
`82`	`69`	`@Override`
`@@ -85,13 +72,6 @@ public String toString()`
`85`	`72`	`return toStringHelper(this)`
`86`	`73`	`.add("schemaTableName", schemaTableName)`
`87`	`74`	`.add("tablePath", tablePath)`
`88`		`- .add("storageType", storageType)`
`89`	`75`	`.toString();`
`90`	`76`	`}`
`91`		`-`
`92`		`- public enum StorageType`
`93`		`- {`
`94`		`- FS, // Local File System`
`95`		`- S3`
`96`		`- }`
`97`	`77`	`}`