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

lirui-apache · 2024-03-15T16:39:34Z

What changes were proposed in this pull request?

Use direct SQL for transactional update table parameter and check the number of affected rows to detect concurrent writes.

Why are the changes needed?

Maintain consistency in case of concurrent writes.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Covered by existing tests

lirui-apache · 2024-03-18T14:26:28Z

@pvary Could you please have a look? I'm unable to reproduce the failed test, and I suppose the no-lock feature is disabled by default.

pvary · 2024-03-18T19:40:31Z

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

+  @Override
+  public int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue)
+      throws MetaException {
+    String dml = String.format(


Could we do something like this:
https://www.datanucleus.org/products/accessplatform_6_0/jdo/query.html#jdoql_bulkupdate

I would like to avoid:

Writing strings to the queries based on table parameters

Using native connection

If using jdo queries doesn't work, then I would create a method in MetaStoreDirectSql for this, and throw an exception if directSql is not turned on

Setting datanucleus.query.jdoql.allowAll=true allows us to run UPDATE with JDO query. I tried something like this but it doesn't work.

Map<String, String> newParams = new HashMap<>(table.getParameters()); newParams.put(key, newValue); openTransaction(); Query query = pm.newQuery("UPDATE org.apache.hadoop.hive.metastore.model.MTable " + "SET parameters=:newparams WHERE database.name == :dbname && tableName == :tblname && " + "parameters.containsEntry(:key, :expval)"); int affectedRows = (int) query.executeWithMap(ImmutableMap.of( "newparams", newParams, "dbname", table.getDbName(), "tblname", table.getTableName(), "key", key, "expval", expectedValue ));

The error is

Caused by: org.datanucleus.store.rdbms.sql.expression.IllegalExpressionOperationException: Cannot perform operation "==" on org.datanucleus.store.rdbms.sql.expression.MapExpression@6f9c5048 and org.datanucleus.store.rdbms.sql.expression.MapLiteral@5114b7c7 at org.datanucleus.store.rdbms.sql.expression.SQLExpression.eq(SQLExpression.java:381) at org.datanucleus.store.rdbms.sql.expression.MapExpression.eq(MapExpression.java:80) at org.datanucleus.store.rdbms.query.QueryToSQLMapper.compileUpdate(QueryToSQLMapper.java:1134)

I believe it's because MapExpression only supports eq with null literals in DN.

That's sad 😢

Then we need to fall back to directsql.
Something like this:

String queryText = "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " + "WHERE \"TBL_ID\" = ? AND \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ?"; try (QueryWrapper query = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", queryText))) { Object[] params = new Object[4]; params[0] = ...; long res = query.executeWithArray(params); ... }

is that ok, if i change code as below?
String queryText = "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " + "WHERE \"TBL_ID\" in (select TBL_ID from TBLS join DBS ON DBS.DB_ID=TBLS.DB_ID WHERE NAME=? and TBL_NAME=? ) AND \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ?"; try (QueryWrapper query = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", queryText))) { Object[] params = new Object[4]; params[0] = ...; long res = query.executeWithArray(params); ... }
cause i don't cherry pick HIVE-22234, can't get table id

Yes, for old versions where Table object doesn't have the ID, I think we need to retrieve it from TBLS. I'm planning to do this in PRs for the release branches.

pvary

LGTM

Let's see if there are any further comments

pvary · 2024-03-26T14:35:55Z

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

+  @Override
+  public int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue)
+      throws MetaException {
+    String dml = String.format(


That's sad 😢

Then we need to fall back to directsql.
Something like this:

String queryText = "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " + "WHERE \"TBL_ID\" = ? AND \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ?"; try (QueryWrapper query = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", queryText))) { Object[] params = new Object[4]; params[0] = ...; long res = query.executeWithArray(params); ... }

lirui-apache · 2024-03-29T07:47:09Z

@pvary Thanks for the reviews. One thing to mention is this change adds a new exception message for commit conflict: The table has been modified. Updating expected key %s affects %d rows. So Iceberg needs to check for this message during commit. Let me know if you think that's not good.

And btw the change has been verified with Postgres, MySQL and MS SQL Server. I didn't manage to get a working Oracle instance.

pvary · 2024-03-30T20:55:35Z

@pvary Thanks for the reviews. One thing to mention is this change adds a new exception message for commit conflict: The table has been modified. Updating expected key %s affects %d rows. So Iceberg needs to check for this message during commit. Let me know if you think that's not good.

And btw the change has been verified with Postgres, MySQL and MS SQL Server. I didn't manage to get a working Oracle instance.

That’s a good point. If possible we should keep the old message, so we do not create even more confusion of versions...

The check there is like this:

        if (e.getMessage()
            .contains(
                "The table has been modified. The parameter value for key '"
                    + HiveTableOperations.METADATA_LOCATION_PROP
                    + "' is")) {
          throw new CommitFailedException(
              e, "The table %s.%s has been modified concurrently", database, tableName);
        }

Either we can issue a query to check the new value, or change it to "the parameter value for key ... is different"

lirui-apache · 2024-04-03T06:56:24Z

@pvary I have made the exception messages consistent. Let me know if you have any further comments.

pvary · 2024-04-04T16:25:45Z

@lirui-apache: we need to fix the ci errors

deniskuzZ · 2024-04-09T08:33:10Z

unrelated test failure, restarted the build

pvary · 2024-04-10T18:49:35Z

Tried to merge using the GitHub UI.
The merge stuck on the UI, but succeeded based on the current head: 7378962

On the UI the PR is still not merged.

@deniskuzZ: Could you please take a look at, if you have some time, to confirm that everything is OK?
Thanks,
Peter

…apache#5129)

lirui-apache · 2024-04-11T08:34:04Z

Thanks @pvary for reviewing and merging the PR. I'm closing it manually.

pvary · 2024-04-11T09:51:58Z

@lirui-apache: Thanks for the report and the fix @lirui-apache!

@deniskuzZ: Which branches are likely to get new releases?

If we want to fix all of the places where the previous PR was released, we might want to add this to:

branch-4.0
branch-3
branch-3.1
branch-2
branch-2.3

deniskuzZ · 2024-04-11T10:22:44Z

hey @pvary
fix is in, looks good! thanks @lirui-apache for the patch

git log --grep "HIVE-28121"

we are planning to release 4.0.1 in a month, not sure about other branches. let me add the proper label to the ticket

lirui-apache · 2024-04-11T10:57:27Z

@lirui-apache: Thanks for the report and the fix @lirui-apache!

@deniskuzZ: Which branches are likely to get new releases?

If we want to fix all of the places where the previous PR was released, we might want to add this to:

branch-4.0

branch-3

branch-3.1

branch-2

branch-2.3

The fix versions of HIVE-26882 are 2.3.10 and 4.0.0-beta-1. Does that mean we don't need this in 3.x?

deniskuzZ · 2024-04-11T13:00:20Z

HIVE-26882 was merged to 3.x and 3.1, so whoever maintains these branches has to cherry-pick

lirui-apache · 2024-04-11T13:18:00Z

@deniskuzZ OK, then I'll create PRs for these branches.

…apache#5129)

chenwyi2 · 2024-04-19T03:18:26Z

when i use this patch, the error is :
Caused by: java.lang.RuntimeException: MetaException(message:You have specified an SQL statement ("UPDATE "TABLE_PARAMS" SET "PARAM_VALUE" = ? WHERE "TBL_ID" in (select TBL_ID from TBLS join DBS ON DBS.DB_ID=TBLS.DB_ID WHERE NAME=? and TBL_NAME=? ) AND "PARAM_KEY" = ? AND "PARAM_VALUE" = ?") that doesnt start with SELECT. This is invalid.)
at org.apache.iceberg.relocated.com.google.common.base.Throwables.propagate(Throwables.java:249)
at org.apache.iceberg.common.DynMethods$UnboundMethod.invoke(DynMethods.java:75)
at org.apache.iceberg.hive.MetastoreUtil.alterTable(MetastoreUtil.java:75)
at org.apache.iceberg.hive.HiveTableOperations.lambda$persistTable$2(HiveTableOperations.java:329)
at org.apache.iceberg.ClientPoolImpl.run(ClientPoolImpl.java:58)

and i use mysql, my code is

    String queryText = "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " + "WHERE \"TBL_ID\" in " +
            "(select TBL_ID from TBLS join DBS ON DBS.DB_ID=TBLS.DB_ID WHERE NAME=? and TBL_NAME=? ) " +
            "AND \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ?";
    List<String> pms = new ArrayList<>();
    pms.add(newValue);
    pms.add(table.getDbName());
    pms.add(table.getTableName());
    pms.add(key);
    pms.add(expectedValue);

    Query queryParams = pm.newQuery("javax.jdo.query.SQL", queryText);
    return (long) executeWithArray(queryParams, pms.toArray(), queryText);

pvary · 2024-04-19T05:09:22Z

@chenwyi2: Could you please share the exception from the metastore side, and your metastore version?

Thanks, Peter

lirui-apache · 2024-04-19T05:25:18Z

@chenwyi2 Could you check whether datanucleus.query.sql.allowAll=true is set on HMS?

chenwyi2 · 2024-04-19T05:35:02Z

@chenwyi2: Could you please share the exception from the metastore side, and your metastore version?

Thanks, Peter

2024-04-19 12:03:06,849 ERROR [pool-9-thread-1]: metastore.RetryingHMSHandler (RetryingHMSHandler.java:invokeInternal(201)) - MetaException(message:JDOQL Single-String query should always start with SELECT)
at org.apache.hadoop.hive.metastore.ObjectStore$GetHelper.handleDirectSqlError(ObjectStore.java:3763)
at org.apache.hadoop.hive.metastore.ObjectStore$GetHelper.run(ObjectStore.java:3717)
at org.apache.hadoop.hive.metastore.ObjectStore.updateParameterWithExpectedValue(ObjectStore.java:775)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.apache.hadoop.hive.metastore.RawStoreProxy.invoke(RawStoreProxy.java:97)
at com.sun.proxy.$Proxy24.updateParameterWithExpectedValue(Unknown Source)
at org.apache.hadoop.hive.metastore.HiveAlterHandler.alterTable(HiveAlterHandler.java:171)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.alter_table_core(HiveMetaStore.java:5262)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.alter_table_with_environment_context(HiveMetaStore.java:5227)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.apache.hadoop.hive.metastore.RetryingHMSHandler.invokeInternal(RetryingHMSHandler.java:147)
at org.apache.hadoop.hive.metastore.RetryingHMSHandler.invoke(RetryingHMSHandler.java:108)
at com.sun.proxy.$Proxy25.alter_table_with_environment_context(Unknown Source)
at org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Processor$alter_table_with_environment_context.getResult(ThriftHiveMetastore.java:15448)
at org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Processor$alter_table_with_environment_context.getResult(ThriftHiveMetastore.java:15432)
at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:39)
at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39)
at org.apache.hadoop.hive.metastore.security.HadoopThriftAuthBridge$Server$TUGIAssumingProcessor$1.run(HadoopThriftAuthBridge.java:636)
at org.apache.hadoop.hive.metastore.security.HadoopThriftAuthBridge$Server$TUGIAssumingProcessor$1.run(HadoopThriftAuthBridge.java:631)
at java.security.AccessController.doPrivileged(Native Method)
at javax.security.auth.Subject.doAs(Subject.java:422)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1762)
at org.apache.hadoop.hive.metastore.security.HadoopThriftAuthBridge$Server$TUGIAssumingProcessor.process(HadoopThriftAuthBridge.java:631)
at org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:286)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)

and i use 3.1.2 in my metastore

chenwyi2 · 2024-04-19T07:29:05Z

sorry it's my fault, i doesn't add parameter in metastore cont just like 7bca1b3, and now it is fine

…apache#5129)

pan3793 · 2024-04-20T16:38:51Z

should this go branch-2.3 too? @pvary @sunchao

pvary · 2024-04-20T16:51:26Z

@pan3793: It takes some time to do all the backports and tests. If you create the PR, I would be happy to review

sunchao · 2024-04-20T18:03:08Z

@pan3793 @pvary I already created a git tag for 2.3.10 RC0: https://github.com/apache/hive/releases/tag/release-2.3.10-rc0. Let me know how important it is to backport this PR. I can wait if you decide to do so.

pan3793 · 2024-04-20T18:39:29Z

to @pvary I opened #5204 to backport it to branch-2.3
to @lirui-apache I don't have the test suite to verify this patch, could you please help to verify it?
to @sunchao from the context, I think this is kind of a correctness issue. I think we can have an RC0 for Spark integration testing, and create RC1 if the Hive community decides to accept this backport.

sunchao · 2024-04-20T20:05:16Z

OK, sounds good. I just created 2.3.10 RC0 and started a vote thread. Please checkout the candidate and see if there is any issue in Spark integration.

cc @dongjoon-hyun too.

dongjoon-hyun · 2024-04-21T03:27:17Z

Thank you, @sunchao and all.

…e parameter (apache#5129) (cherry picked from commit 7378962)

…from 2.3.10 into ODP hive-2.3.101 and merge into 2.3.102 branch (#28) * ODP-1794 | HIVE-7145: Remove dependence on apache commons-lang (David Lavati via László Bodor, Zoltan Haindrich) Signed-off-by: Zoltan Haindrich <kirk@rxd.hu> (cherry picked from commit 50296ef) * HIVE-20016: Investigate TestJdbcWithMiniHS2.testParallelCompilation3 random failure (Yongzhi Chen, reviewed by Aihua Xu) (cherry picked from commit 3b6d4e2) * ODP-1794 | HIVE-28121: Use direct SQL for transactional altering table parameter (apache#5129) (cherry picked from commit 7378962) * ODP-1794 Chnaged libthrift version to 0.16.0 Signed-off-by: Zoltan Haindrich <kirk@rxd.hu> (cherry picked from commit 50296ef) * Updated Hive pom from 2.3.101 to 2.3.102 * ODP-1794 Changed hive version from 2.3.101 to 2.3.102 and ivy version to 2.5.2 --------- Co-authored-by: David Lavati <david.lavati@gmail.com> Co-authored-by: Yongzhi Chen <ychena@apache.org> Co-authored-by: Rui Li <lirui@apache.org> Co-authored-by: Shubham Sharma <shubh.luck@yahoo.in>

asf-ci-hive added tests pending tests unstable and removed tests pending labels Mar 15, 2024

pvary reviewed Mar 18, 2024

View reviewed changes

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Mar 28, 2024

pvary approved these changes Mar 28, 2024

View reviewed changes

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending labels Apr 1, 2024

HonahX mentioned this pull request Apr 6, 2024

Concern about possible consistency issue in HiveCatalog's _commit_table apache/iceberg-python#588

Closed

asf-ci-hive added tests pending and removed tests unstable labels Apr 9, 2024

lirui-apache added 3 commits April 9, 2024 17:07

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

60e09d6

use MetaStoreDirectSql

a0a0890

change exception msg

54a62df

lirui-apache force-pushed the HIVE-28121 branch from ae097b6 to 54a62df Compare April 9, 2024 09:07

asf-ci-hive added tests failed tests pending and removed tests pending tests failed labels Apr 9, 2024

asf-ci-hive added the tests passed label Apr 10, 2024

ngsg pushed a commit to ngsg/hive that referenced this pull request Apr 11, 2024

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

7378962

…apache#5129)

lirui-apache closed this Apr 11, 2024

lirui-apache deleted the HIVE-28121 branch April 16, 2024 06:49

lirui-apache added a commit to lirui-apache/hive that referenced this pull request Apr 16, 2024

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

384010b

…apache#5129)

lirui-apache mentioned this pull request Apr 16, 2024

HIVE-28121: Use direct SQL for transactional altering table parameter (#5129) #5197

Merged

lirui-apache added a commit to lirui-apache/hive that referenced this pull request Apr 19, 2024

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

60fc152

…apache#5129)

lirui-apache mentioned this pull request Apr 19, 2024

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

Merged

pan3793 mentioned this pull request Apr 20, 2024

HIVE-28121. (2.3) Use direct SQL for transactional altering table parameter #5204

Merged

lirui-apache mentioned this pull request Apr 22, 2024

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

Merged

senthh pushed a commit to acceldata-io/hive that referenced this pull request Jul 11, 2024

ODP-1794 | HIVE-28121: Use direct SQL for transactional altering tabl…

5f3a6f6

…e parameter (apache#5129) (cherry picked from commit 7378962)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lirui-apache commented Mar 15, 2024

lirui-apache commented Mar 18, 2024

pvary Mar 18, 2024

lirui-apache Mar 25, 2024

pvary Mar 26, 2024

chenwyi2 Apr 12, 2024

lirui-apache Apr 12, 2024

pvary left a comment

pvary Mar 26, 2024

lirui-apache commented Mar 29, 2024

pvary commented Mar 30, 2024

lirui-apache commented Apr 3, 2024

pvary commented Apr 4, 2024

deniskuzZ commented Apr 9, 2024

pvary commented Apr 10, 2024

lirui-apache commented Apr 11, 2024

pvary commented Apr 11, 2024

deniskuzZ commented Apr 11, 2024 •

edited

Loading

lirui-apache commented Apr 11, 2024

deniskuzZ commented Apr 11, 2024

lirui-apache commented Apr 11, 2024

chenwyi2 commented Apr 19, 2024

pvary commented Apr 19, 2024

lirui-apache commented Apr 19, 2024

chenwyi2 commented Apr 19, 2024

chenwyi2 commented Apr 19, 2024

pan3793 commented Apr 20, 2024

pvary commented Apr 20, 2024

sunchao commented Apr 20, 2024

pan3793 commented Apr 20, 2024 •

edited

Loading

sunchao commented Apr 20, 2024

dongjoon-hyun commented Apr 21, 2024

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

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

Conversation

lirui-apache commented Mar 15, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

lirui-apache commented Mar 18, 2024

pvary Mar 18, 2024

Choose a reason for hiding this comment

lirui-apache Mar 25, 2024

Choose a reason for hiding this comment

pvary Mar 26, 2024

Choose a reason for hiding this comment

chenwyi2 Apr 12, 2024

Choose a reason for hiding this comment

lirui-apache Apr 12, 2024

Choose a reason for hiding this comment

pvary left a comment

Choose a reason for hiding this comment

pvary Mar 26, 2024

Choose a reason for hiding this comment

lirui-apache commented Mar 29, 2024

pvary commented Mar 30, 2024

lirui-apache commented Apr 3, 2024

pvary commented Apr 4, 2024

deniskuzZ commented Apr 9, 2024

pvary commented Apr 10, 2024

lirui-apache commented Apr 11, 2024

pvary commented Apr 11, 2024

deniskuzZ commented Apr 11, 2024 • edited Loading

lirui-apache commented Apr 11, 2024

deniskuzZ commented Apr 11, 2024

lirui-apache commented Apr 11, 2024

chenwyi2 commented Apr 19, 2024

pvary commented Apr 19, 2024

lirui-apache commented Apr 19, 2024

chenwyi2 commented Apr 19, 2024

chenwyi2 commented Apr 19, 2024

pan3793 commented Apr 20, 2024

pvary commented Apr 20, 2024

sunchao commented Apr 20, 2024

pan3793 commented Apr 20, 2024 • edited Loading

sunchao commented Apr 20, 2024

dongjoon-hyun commented Apr 21, 2024

deniskuzZ commented Apr 11, 2024 •

edited

Loading

pan3793 commented Apr 20, 2024 •

edited

Loading