HIVE-28121: Use direct SQL for transactional altering table parameter

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java

@@ -152,23 +152,28 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam
       String expectedValue = environmentContext != null && environmentContext.getProperties() != null ?
               environmentContext.getProperties().get(hive_metastoreConstants.EXPECTED_PARAMETER_VALUE) : null;
 
-      if (expectedKey != null) {
-        // If we have to check the expected state of the table we have to prevent nonrepeatable reads.
-        msdb.openTransaction(Constants.TX_REPEATABLE_READ);
-      } else {
-        msdb.openTransaction();
-      }
+      msdb.openTransaction();
       // get old table
       oldt = msdb.getTable(catName, dbname, name);
       if (oldt == null) {
         throw new InvalidOperationException("table " +
             Warehouse.getCatalogQualifiedTableName(catName, dbname, name) + " doesn't exist");
       }
 
-      if (expectedKey != null && expectedValue != null
-              && !expectedValue.equals(oldt.getParameters().get(expectedKey))) {
-        throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '"
-                + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'");
+      if (expectedKey != null && expectedValue != null) {
+        String newValue = newt.getParameters().get(expectedKey);
+        if (newValue == null) {
+          throw new MetaException(String.format("New value for expected key %s is not set", expectedKey));
+        }
+        if (!expectedValue.equals(oldt.getParameters().get(expectedKey))) {
+          throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '"
+              + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'");
+        }
+        long affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue);
+        if (affectedRows != 1) {
+          // make sure concurrent modification exception messages have the same prefix
+          throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is different");
+        }
       }
 
       if (oldt.getPartitionKeysSize() != 0) {

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java

@@ -2555,4 +2555,13 @@ private void getStatsTableListResult(
       query.closeAll();
     }
   }
+
+  long updateTableParam(Table table, String key, String expectedValue, String newValue) {
+    String queryText = String.format("UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " +
+        "WHERE \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ? AND \"TBL_ID\" IN " +
+        "(SELECT \"TBL_ID\" FROM %s JOIN %s ON %s.\"DB_ID\" = %s.\"DB_ID\" WHERE \"TBL_NAME\" = '%s' AND \"NAME\" = '%s')",
+        TBLS, DBS, TBLS, DBS, table.getTableName(), table.getDbName());
+    Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
+    return (long) query.executeWithArray(newValue, key, expectedValue);
+  }
 }

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

@@ -751,6 +751,29 @@ public boolean openTransaction(String isolationLevel) {
     return result;
   }
 
+  @Override
+  public long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue)
+      throws MetaException, NoSuchObjectException {
+    return new GetHelper<Long>(table.getCatName(), table.getDbName(), table.getTableName(), true, false) {
+
+      @Override
+      protected String describeResult() {
+        return "Affected rows";
+      }
+
+      @Override
+      protected Long getSqlResult(GetHelper<Long> ctx) throws MetaException {
+        return directSql.updateTableParam(table, key, expectedValue, newValue);
+      }
+
+      @Override
+      protected Long getJdoResult(GetHelper<Long> ctx) throws MetaException, NoSuchObjectException {
+        throw new UnsupportedOperationException(
+            "Cannot update parameter with JDO, make sure direct SQL is enabled");
+      }
+    }.run(false);
+  }
+
   /**
    * if this is the commit of the first open call then an actual commit is
    * called.

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java

@@ -1657,4 +1657,13 @@ List<SchemaVersion> getSchemaVersionsByColumns(String colName, String colNamespa
   Map<String, List<String>> getPartitionColsWithStats(String catName, String dbName,
       String tableName) throws MetaException, NoSuchObjectException;
 
+  /**
+   * Updates a given table parameter with expected value.
+   *
+   * @return the number of rows updated
+   */
+  default long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue)
+      throws MetaException, NoSuchObjectException {
+    throw new UnsupportedOperationException("This Store doesn't support updating table parameter with expected value");
+  }
 }

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java

@@ -419,6 +419,10 @@ public enum ConfVars {
         "Default transaction isolation level for identity generation."),
     DATANUCLEUS_USE_LEGACY_VALUE_STRATEGY("datanucleus.rdbms.useLegacyNativeValueStrategy",
         "datanucleus.rdbms.useLegacyNativeValueStrategy", true, ""),
+    DATANUCLEUS_QUERY_SQL_ALLOWALL("datanucleus.query.sql.allowAll", "datanucleus.query.sql.allowAll",
+        true, "In strict JDO all SQL queries must begin with \"SELECT ...\", and consequently it "
+        + "is not possible to execute queries that change data. This DataNucleus property when set to true allows "
+        + "insert, update and delete operations from JDO SQL. Default value is true."),
     DBACCESS_SSL_PROPS("metastore.dbaccess.ssl.properties", "hive.metastore.dbaccess.ssl.properties", "",
         "Comma-separated SSL properties for metastore to access database when JDO connection URL\n" +
             "enables SSL access. e.g. javax.net.ssl.trustStore=/tmp/truststore,javax.net.ssl.trustStorePassword=pwd."),
@@ -1108,6 +1112,7 @@ public String toString() {
       ConfVars.DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK,
       ConfVars.DATANUCLEUS_TRANSACTION_ISOLATION,
       ConfVars.DATANUCLEUS_USE_LEGACY_VALUE_STRATEGY,
+      ConfVars.DATANUCLEUS_QUERY_SQL_ALLOWALL,
       ConfVars.DETACH_ALL_ON_COMMIT,
       ConfVars.IDENTIFIER_FACTORY,
       ConfVars.MANAGER_FACTORY_CLASS,

standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java

@@ -49,13 +49,15 @@
 import org.apache.hadoop.hive.metastore.client.builder.PartitionBuilder;
 import org.apache.hadoop.hive.metastore.client.builder.TableBuilder;
 import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars;
 import org.apache.hadoop.hive.metastore.minihms.AbstractMetaStoreService;
 import org.apache.thrift.TApplicationException;
 import org.apache.thrift.TException;
 import org.apache.thrift.protocol.TProtocolException;
 import org.apache.thrift.transport.TTransportException;
 import org.junit.After;
 import org.junit.Assert;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -105,7 +107,7 @@ public TestTablesCreateDropAlterTruncate(String name, AbstractMetaStoreService m
 
   @BeforeClass
   public static void startMetaStores() {
-    Map<MetastoreConf.ConfVars, String> msConf = new HashMap<MetastoreConf.ConfVars, String>();
+    Map<ConfVars, String> msConf = new HashMap<ConfVars, String>();
     // Enable trash, so it can be tested
     Map<String, String> extraConf = new HashMap<>();
     extraConf.put("fs.trash.checkpoint.interval", "30");  // FS_TRASH_CHECKPOINT_INTERVAL_KEY
@@ -1086,6 +1088,8 @@ public void testAlterTableAlreadyExists() throws Exception {
 
   @Test
   public void testAlterTableExpectedPropertyMatch() throws Exception {
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL));
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL));
     Table originalTable = testTables[0];
 
     EnvironmentContext context = new EnvironmentContext();
@@ -1099,6 +1103,8 @@ public void testAlterTableExpectedPropertyMatch() throws Exception {
 
   @Test(expected = MetaException.class)
   public void testAlterTableExpectedPropertyDifferent() throws Exception {
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL));
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL));
     Table originalTable = testTables[0];
 
     EnvironmentContext context = new EnvironmentContext();
@@ -1118,6 +1124,8 @@ public void testAlterTableExpectedPropertyDifferent() throws Exception {
    */
   @Test
   public void testAlterTableExpectedPropertyConcurrent() throws Exception {
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL));
+    Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL));
     Table originalTable = testTables[0];
 
     originalTable.getParameters().put("snapshot", "0");