This is an automated email from the ASF dual-hosted git repository.
pvary pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new c0250c745d Hive: Fix lock selection during table creation to respect
table properties (#14236)
c0250c745d is described below
commit c0250c745d29ddcf892a06294a35019762f51686
Author: s-sanjay <[email protected]>
AuthorDate: Mon Oct 20 16:07:47 2025 +0530
Hive: Fix lock selection during table creation to respect table properties
(#14236)
---
.../apache/iceberg/hive/HiveTableOperations.java | 2 +-
.../org/apache/iceberg/hive/TestHiveCommits.java | 52 ++++++++++++++++++
.../org/apache/iceberg/hive/TestHiveTable.java | 61 ++++++++++++++++++++++
3 files changed, 114 insertions(+), 1 deletion(-)
diff --git
a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
index f0183fc5c5..3e4be78169 100644
---
a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
+++
b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
@@ -143,7 +143,7 @@ public class HiveTableOperations extends
BaseMetastoreTableOperations
BaseMetastoreOperations.CommitStatus.FAILURE;
boolean updateHiveTable = false;
- HiveLock lock = lockObject(base);
+ HiveLock lock = lockObject(base != null ? base : metadata);
try {
lock.lock();
diff --git
a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
index 399cead0cb..f78bb5d3e7 100644
--- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
+++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
@@ -22,6 +22,7 @@ import static
org.apache.iceberg.TableProperties.HIVE_LOCK_ENABLED;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.doAnswer;
@@ -45,6 +46,9 @@ import
org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.apache.thrift.TException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
import org.junit.platform.commons.support.ReflectionSupport;
public class TestHiveCommits extends HiveTableTestBase {
@@ -504,6 +508,54 @@ public class TestHiveCommits extends HiveTableTestBase {
.hasSameClassAs(initialLock);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ @NullSource
+ public void testFirstHiveCommitWithLockSetting(Boolean lockEnabled) {
+ TableIdentifier newTableIdentifier = TableIdentifier.of(DB_NAME,
"lock_test_table");
+
+ try {
+ HiveTableOperations ops =
+ new HiveTableOperations(
+ catalog.getConf(),
+ catalog.clientPool(),
+ catalog.newTableOps(newTableIdentifier).io(),
+ catalog.name(),
+ newTableIdentifier.namespace().level(0),
+ newTableIdentifier.name());
+
+ AtomicReference<HiveLock> lockRef = new AtomicReference<>();
+ HiveTableOperations spyOps = spy(ops);
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ catalog.defaultWarehouseLocation(newTableIdentifier),
+ lockEnabled == null
+ ? ImmutableMap.of()
+ : ImmutableMap.of(HIVE_LOCK_ENABLED,
String.valueOf(lockEnabled)));
+ doAnswer(
+ i -> {
+ lockRef.set(ops.lockObject(i.getArgument(0)));
+ return lockRef.get();
+ })
+ .when(spyOps)
+ .lockObject(eq(metadata));
+
+ // Commit with base = null (new table creation)
+ spyOps.commit(null, metadata);
+
+ Class<? extends HiveLock> expectedLockClass =
+ Boolean.FALSE.equals(lockEnabled) ? NoLock.class :
MetastoreLock.class;
+ assertThat(lockRef).as("Lock not captured by the
stub").doesNotHaveNullValue();
+ assertThat(lockRef)
+ .as("Lock mechanism should use (%s)",
expectedLockClass.getSimpleName())
+ .hasValueMatching(lock -> lock.getClass().equals(expectedLockClass));
+ } finally {
+ catalog.dropTable(newTableIdentifier, true);
+ }
+ }
+
private void commitAndThrowException(
HiveTableOperations realOperations, HiveTableOperations spyOperations)
throws TException, InterruptedException {
diff --git
a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveTable.java
b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveTable.java
index 031015ec5a..9c6a2b216b 100644
--- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveTable.java
+++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveTable.java
@@ -26,10 +26,14 @@ import static
org.apache.iceberg.BaseMetastoreTableOperations.METADATA_LOCATION_
import static
org.apache.iceberg.BaseMetastoreTableOperations.PREVIOUS_METADATA_LOCATION_PROP;
import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP;
import static org.apache.iceberg.TableMetadataParser.getFileExtension;
+import static org.apache.iceberg.TableProperties.HIVE_LOCK_ENABLED;
import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.spy;
import java.io.File;
import java.io.IOException;
@@ -39,6 +43,7 @@ import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
import org.apache.avro.generic.GenericData;
import org.apache.avro.generic.GenericRecordBuilder;
@@ -60,7 +65,9 @@ import org.apache.iceberg.ManifestFile;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
+import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.TableOperations;
import org.apache.iceberg.TableProperties;
import org.apache.iceberg.avro.Avro;
import org.apache.iceberg.avro.AvroSchemaUtil;
@@ -74,6 +81,7 @@ import org.apache.iceberg.exceptions.NotFoundException;
import org.apache.iceberg.hadoop.ConfigProperties;
import org.apache.iceberg.hadoop.HadoopCatalog;
import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types;
@@ -82,6 +90,8 @@ import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
public class TestHiveTable extends HiveTableTestBase {
static final String NON_DEFAULT_DATABASE = "nondefault";
@@ -119,6 +129,57 @@ public class TestHiveTable extends HiveTableTestBase {
assertThat(icebergTable.schema().asStruct()).isEqualTo(SCHEMA.asStruct());
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ @NullSource
+ public void testCreateTableEndToEnd(Boolean lockEnabled) {
+ TableIdentifier newTableIdentifier = TableIdentifier.of(DB_NAME,
"lock_test_table");
+
+ try {
+ // Create table with lock setting via catalog
+ // We will spy the call to create table operation and capture the lock
object being returned
+ HiveCatalog spyCatalog = spy(catalog);
+ AtomicReference<HiveLock> lockRef = new AtomicReference<>();
+ doAnswer(
+ (args) -> {
+ TableOperations ops =
+ catalog.newTableOps(args.getArgument(0,
TableIdentifier.class));
+ HiveTableOperations spyOps = (HiveTableOperations) spy(ops);
+ doAnswer(
+ (lockArgs) -> {
+ lockRef.set(
+ ((HiveTableOperations) ops)
+ .lockObject(lockArgs.getArgument(0,
TableMetadata.class)));
+ return lockRef.get();
+ })
+ .when(spyOps)
+ .lockObject(any());
+ return spyOps;
+ })
+ .when(spyCatalog)
+ .newTableOps(newTableIdentifier);
+
+ spyCatalog.createTable(
+ newTableIdentifier,
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ lockEnabled == null
+ ? ImmutableMap.of()
+ : ImmutableMap.of(HIVE_LOCK_ENABLED,
String.valueOf(lockEnabled)));
+
+ Class<? extends HiveLock> expectedLockClass =
+ Boolean.FALSE.equals(lockEnabled) ? NoLock.class :
MetastoreLock.class;
+ assertThat(lockRef).as("Lock not captured by the
stub").doesNotHaveNullValue();
+ assertThat(lockRef)
+ .as(
+ "Table %s created with HIVE_LOCK_ENABLED=%s should be created
with lock class (%s)",
+ newTableIdentifier, lockEnabled,
expectedLockClass.getSimpleName())
+ .hasValueMatching(lock -> lock.getClass().equals(expectedLockClass));
+ } finally {
+ catalog.dropTable(newTableIdentifier, true);
+ }
+ }
+
@Test
public void testRename() {
String renamedTableName = "rename_table_name";
