PHOENIX-7948 Transform ITs request illegal transform schema options for mutable tables

phoenix-core-client/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java

@@ -784,6 +784,9 @@ public SQLException newException(SQLExceptionInfo info) {
 
   CANNOT_TRANSFORM_TRANSACTIONAL_TABLE(914, "43M25", "Cannot transform a transactional table."),
 
+  CANNOT_TRANSFORM_MUTABLE_TABLE_TO_SCAWO(917, "43M28",
+    "IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS is incompatible with mutable rows."),
+
   STALE_METADATA_CACHE_EXCEPTION(915, "43M26", "Stale metadata cache exception",
     info -> new StaleMetadataCacheException(info.getMessage())),
 

phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMaintainer.java

@@ -1685,7 +1685,12 @@ private boolean hasIndexedColumnChanged(ValueGetter oldState,
   }
 
   public Delete buildRowDeleteMutation(byte[] indexRowKey, DeleteType deleteType, long ts) {
-    byte[] emptyCF = emptyKeyValueCFPtr.copyBytesIfNecessary();
+    // Use the accessors rather than the fields directly. TransformMaintainer shadows
+    // emptyKeyValueCFPtr and coveredColumnsMap with its own fields and overrides
+    // getEmptyKeyValueFamily()/getCoveredColumnsMap(), so reading the fields here would
+    // dereference the never initialized IndexMaintainer copies and NPE.
+    byte[] emptyCF = getEmptyKeyValueFamily().copyBytesIfNecessary();
+    Map<ColumnReference, ColumnReference> coveredColumnsMap = getCoveredColumnsMap();
     Delete delete = new Delete(indexRowKey);
 
     for (ColumnReference ref : getCoveredColumns()) {
@@ -1737,6 +1742,7 @@ public Delete buildDeleteMutation(KeyValueBuilder kvBuilder, ValueGetter oldStat
       return buildRowDeleteMutation(indexRowKey, deleteType, ts);
     }
     Delete delete = null;
+    Map<ColumnReference, ColumnReference> coveredColumnsMap = getCoveredColumnsMap();
     Set<ColumnReference> dataTableColRefs = coveredColumnsMap.keySet();
     // Delete columns for missing key values
     for (Cell kv : pendingUpdates) {
@@ -1770,6 +1776,16 @@ public Set<ColumnReference> getCoveredColumns() {
     return coveredColumnsMap.keySet();
   }
 
+  /**
+   * Returns the map from data table column references to their counterparts in the maintained
+   * table. TransformMaintainer shadows {@link #coveredColumnsMap} with its own field, so callers in
+   * code paths shared with TransformMaintainer must use this accessor rather than reading the field
+   * directly to avoid dereferencing the uninitialized copy.
+   */
+  protected Map<ColumnReference, ColumnReference> getCoveredColumnsMap() {
+    return coveredColumnsMap;
+  }
+
   public Set<ColumnReference> getAllColumns() {
     return allColumns;
   }

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

@@ -4923,9 +4923,9 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
           /**
            * To check if TTL is defined at any of the child below we are checking it at
            * {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl#mutateColumn(List, ColumnMutator, int, PTable, PTable, boolean)}
-           * level where in function
-           * {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl# validateIfMutationAllowedOnParent(PTable, List, PTableType, long, byte[], byte[], byte[], List, int)}
-           * we are already traversing through allDescendantViews.
+           * level where in function {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl#
+           * validateIfMutationAllowedOnParent(PTable, List, PTableType, long, byte[], byte[],
+           * byte[], List, int)} we are already traversing through allDescendantViews.
            */
         }
 
@@ -4947,6 +4947,22 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
             throw new SQLExceptionInfo.Builder(CANNOT_TRANSFORM_TRANSACTIONAL_TABLE)
               .setSchemaName(schemaName).setTableName(tableName).build().buildException();
           }
+          // SINGLE_CELL_ARRAY_WITH_OFFSETS is only valid for immutable tables. Rather than
+          // silently downgrading the requested storage scheme to ONE_CELL_PER_COLUMN, reject the
+          // transform unless the table is already immutable or is being made immutable in this
+          // same ALTER TABLE statement.
+          boolean willBeImmutableForScheme =
+            Boolean.TRUE.equals(metaPropertiesEvaluated.getIsImmutableRows())
+              || (metaPropertiesEvaluated.getIsImmutableRows() == null && table.isImmutableRows());
+          if (
+            table.getType() == TABLE
+              && metaProperties.getImmutableStorageSchemeProp() == SINGLE_CELL_ARRAY_WITH_OFFSETS
+              && !willBeImmutableForScheme
+          ) {
+            throw new SQLExceptionInfo.Builder(
+              SQLExceptionCode.CANNOT_TRANSFORM_MUTABLE_TABLE_TO_SCAWO).setSchemaName(schemaName)
+                .setTableName(tableName).build().buildException();
+          }
         }
 
         // If changing isImmutableRows to true or it's not being changed and is already true

phoenix-core-client/src/main/java/org/apache/phoenix/schema/transform/TransformMaintainer.java

@@ -133,6 +133,11 @@ public Set<ColumnReference> getCoveredColumns() {
     return coveredColumnsMap.keySet();
   }
 
+  @Override
+  protected Map<ColumnReference, ColumnReference> getCoveredColumnsMap() {
+    return coveredColumnsMap;
+  }
+
   private TransformMaintainer(final PTable oldTable, final PTable newTable,
     PhoenixConnection connection) {
     this(oldTable.getRowKeySchema(), oldTable.getBucketNum() != null);

phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexTwoPhaseCreateIT.java

@@ -35,6 +35,7 @@
 import java.util.Properties;
 import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants;
 import org.apache.phoenix.end2end.IndexToolIT;
+import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest;
 import org.apache.phoenix.end2end.transform.TransformToolIT;
 import org.apache.phoenix.jdbc.PhoenixConnection;
 import org.apache.phoenix.query.BaseTest;
@@ -50,9 +51,11 @@
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.junit.experimental.categories.Category;
 
 import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
 
+@Category(NeedsOwnMiniClusterTest.class)
 public class IndexTwoPhaseCreateIT extends BaseTest {
   @BeforeClass
   public static synchronized void doSetup() throws Exception {
@@ -131,9 +134,12 @@ public void testTransformingTableAndIndex() throws Exception {
       String tableName = "TBL_" + generateUniqueName();
       String idxName = "IND_" + generateUniqueName();
 
+      // The table must be immutable: SINGLE_CELL_ARRAY_WITH_OFFSETS packs all of a row's
+      // columns into a single cell, which is incompatible with in-place mutation.
       String createTableSql =
         "CREATE TABLE " + tableName + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
-          + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) ";
+          + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) "
+          + "IMMUTABLE_ROWS=true";
       conn.createStatement().execute(createTableSql);
 
       String upsertStmt =

phoenix-core/src/it/java/org/apache/phoenix/end2end/transform/TransformIT.java

@@ -18,6 +18,7 @@
 package org.apache.phoenix.end2end.transform;
 
 import static org.apache.phoenix.exception.SQLExceptionCode.CANNOT_TRANSFORM_LOCAL_OR_VIEW_INDEX;
+import static org.apache.phoenix.exception.SQLExceptionCode.CANNOT_TRANSFORM_MUTABLE_TABLE_TO_SCAWO;
 import static org.apache.phoenix.exception.SQLExceptionCode.CANNOT_TRANSFORM_TABLE_WITH_LOCAL_INDEX;
 import static org.apache.phoenix.exception.SQLExceptionCode.VIEW_WITH_PROPERTIES;
 import static org.apache.phoenix.schema.PTable.ImmutableStorageScheme.ONE_CELL_PER_COLUMN;
@@ -36,6 +37,7 @@
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.Properties;
+import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest;
 import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
 import org.apache.phoenix.end2end.index.SingleCellIndexIT;
 import org.apache.phoenix.exception.SQLExceptionCode;
@@ -50,9 +52,11 @@
 import org.apache.phoenix.util.SchemaUtil;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.experimental.categories.Category;
 
 import org.apache.phoenix.thirdparty.com.google.common.base.Strings;
 
+@Category(NeedsOwnMiniClusterTest.class)
 public class TransformIT extends ParallelStatsDisabledIT {
   private Properties testProps = PropertiesUtil.deepCopy(TEST_PROPERTIES);
 
@@ -131,7 +135,8 @@ public void testSystemTransformTablePopulatedForTable() throws Exception {
 
       String createTableSql = "CREATE TABLE " + tableName
         + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
-        + "V1 VARCHAR, V2 INTEGER, V3 INTEGER, V4 VARCHAR, V5 VARCHAR CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) ";
+        + "V1 VARCHAR, V2 INTEGER, V3 INTEGER, V4 VARCHAR, V5 VARCHAR CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) "
+        + "IMMUTABLE_ROWS=true";
       conn.createStatement().execute(createTableSql);
 
       assertMetadata(conn, ONE_CELL_PER_COLUMN, NON_ENCODED_QUALIFIERS, tableName);
@@ -267,6 +272,7 @@ public void testTransformForLiveMutations_mutatingMutableTable() throws Exceptio
   }
 
   private void testTransformForLiveMutations_mutatingTable(String tableDDL) throws Exception {
+    boolean isImmutable = tableDDL.toUpperCase().contains("IMMUTABLE_ROWS=TRUE");
     try (Connection conn = DriverManager.getConnection(getUrl(), testProps)) {
       conn.setAutoCommit(true);
       String schema = "S_" + generateUniqueName();
@@ -296,6 +302,19 @@ private void testTransformForLiveMutations_mutatingTable(String tableDDL) throws
       conn.createStatement()
         .execute("CREATE INDEX " + idxName2 + " ON " + fullTableName + " (V1) include (V2) ASYNC");
 
+      if (!isImmutable) {
+        // SINGLE_CELL_ARRAY_WITH_OFFSETS is incompatible with mutable rows, so the transform
+        // should be rejected instead of silently downgraded.
+        try {
+          conn.createStatement().execute("ALTER TABLE " + fullTableName + " SET "
+            + " IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2");
+          fail("Transforming a mutable table to SINGLE_CELL_ARRAY_WITH_OFFSETS should fail");
+        } catch (SQLException e) {
+          assertEquals(CANNOT_TRANSFORM_MUTABLE_TABLE_TO_SCAWO.getErrorCode(), e.getErrorCode());
+        }
+        return;
+      }
+
       // Now do a transform and check still the index table is empty since we didn't build it
       conn.createStatement().execute("ALTER TABLE " + fullTableName + " SET "
         + " IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2");
@@ -331,9 +350,9 @@ public void testTransformForLiveMutations_mutatingBaseTableNoIndex() throws Exce
       String tableName = "TBL_" + generateUniqueName();
       String fullTableName = SchemaUtil.getTableName(schema, tableName);
 
-      String createTableSql =
-        "CREATE TABLE " + fullTableName + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
-          + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) ";
+      String createTableSql = "CREATE TABLE " + fullTableName
+        + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
+        + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) IMMUTABLE_ROWS=true";
       conn.createStatement().execute(createTableSql);
       assertMetadata(conn, ONE_CELL_PER_COLUMN, NON_ENCODED_QUALIFIERS, fullTableName);
 
@@ -427,6 +446,7 @@ public void testTransformForLiveMutations_mutatingImmutableBaseTableForView() th
 
   private void testTransformForLiveMutations_mutatingBaseTableForView(String tableDDL)
     throws Exception {
+    boolean isImmutable = tableDDL.toUpperCase().contains("IMMUTABLE_ROWS=TRUE");
     try (Connection conn = DriverManager.getConnection(getUrl(), testProps)) {
       conn.setAutoCommit(true);
       String schema = "S_" + generateUniqueName();
@@ -458,6 +478,19 @@ private void testTransformForLiveMutations_mutatingBaseTableForView(String table
         + " (PK1, INT_PK, V1, VIEW_COL1, VIEW_COL2) VALUES ('%s', %d, '%s', %d, '%s')";
       conn.createStatement().execute(String.format(upsertStmt, "a", 1, "val1", 1, "col2_1"));
 
+      if (!isImmutable) {
+        // SINGLE_CELL_ARRAY_WITH_OFFSETS is incompatible with mutable rows, so the transform
+        // should be rejected instead of silently downgraded.
+        try {
+          conn.createStatement().execute("ALTER TABLE " + fullTableName + " SET "
+            + " IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2");
+          fail("Transforming a mutable table to SINGLE_CELL_ARRAY_WITH_OFFSETS should fail");
+        } catch (SQLException e) {
+          assertEquals(CANNOT_TRANSFORM_MUTABLE_TABLE_TO_SCAWO.getErrorCode(), e.getErrorCode());
+        }
+        return;
+      }
+
       conn.createStatement().execute("ALTER TABLE " + fullTableName + " SET "
         + " IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2");
       SystemTransformRecord record = Transform.getTransformRecord(schema, tableName, null, null,
@@ -523,7 +556,7 @@ public void testAlterNotNeedsToTransformDueToSameProps() throws Exception {
       String createTableSql = "CREATE TABLE " + fullTableName
         + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
         + "V1 VARCHAR, V2 INTEGER, V3 INTEGER, V4 VARCHAR, V5 VARCHAR CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) "
-        + "IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2";
+        + "IMMUTABLE_ROWS=true, IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2";
       conn.createStatement().execute(createTableSql);
       assertMetadata(conn, PTable.ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS,
         PTable.QualifierEncodingScheme.TWO_BYTE_QUALIFIERS, fullTableName);
@@ -561,9 +594,9 @@ public void testDropAfterTransform() throws Exception {
     try (Connection conn = DriverManager.getConnection(getUrl(), testProps)) {
       conn.setAutoCommit(true);
 
-      String createTableSql =
-        "CREATE TABLE " + fullTableName + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
-          + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) ";
+      String createTableSql = "CREATE TABLE " + fullTableName
+        + " (PK1 VARCHAR NOT NULL, INT_PK INTEGER NOT NULL, "
+        + "V1 VARCHAR, V2 INTEGER CONSTRAINT NAME_PK PRIMARY KEY(PK1, INT_PK)) IMMUTABLE_ROWS=true";
       conn.createStatement().execute(createTableSql);
       assertMetadata(conn, ONE_CELL_PER_COLUMN, NON_ENCODED_QUALIFIERS, fullTableName);