Hive insert overwrite when using S3 storage #17307

nmahadevuni · 2022-02-16T13:07:58Z

Co-authored-by: Arkadiusz Czajkowski arek@starburstdata.com

This PR fixes insert overwrite operation on S3 backing storage when below property is set

hive.insert-existing-partitions-behavior=OVERWRITE

Applied fix writes to partition target directory directly. After successfully writing new files coordinator deletes all files within partition whose name prefix or suffix didn't match current query ID

Test plan - Test containers for HMS and MinIO allowing to test and debug S3 related issue easily from IDE
based on above local dockerized S3 data lake

== RELEASE NOTES ==

Hive Changes

*  Support for overwriting existing partitions with a hive config property `hive.insert-existing-partitions-behavior`. The config supersedes a legacy config `hive.insert-overwrite-immutable-partitions-enabled`. The new config adds capability of overwriting new partitions for S3.

nmahadevuni · 2022-02-18T14:00:34Z

@jainxrohit Can you please review this?

NikhilCollooru · 2022-03-14T17:28:52Z

I think the problem is that S3 is not deleting the files of a partition after its dropped. Both this PR and #17369 are trying to solve the same problems in different ways.

I feel #17369 is the better approach since it fixes the drop partition for S3.

nmahadevuni · 2022-03-14T17:50:34Z

I think the problem is that S3 is not deleting the files of a partition after its dropped. Both this PR and #17369 are trying to solve the same problems in different ways.

I feel #17369 is the better approach since it fixes the drop partition for S3.

@NikhilCollooru : Sorry, both these PRs are not the solving same problem. This PR is to support INSERT(overwrite) into existing partition on S3. It doesn't fix anything related to drop partition.

NikhilCollooru · 2022-03-14T20:26:43Z

I think the problem is that S3 is not deleting the files of a partition after its dropped. Both this PR and #17369 are trying to solve the same problems in different ways.
I feel #17369 is the better approach since it fixes the drop partition for S3.

@NikhilCollooru : Sorry, both these PRs are not the solving same problem. This PR is to support INSERT(overwrite) into existing partition on S3. It doesn't fix anything related to drop partition.

Okay . I see it now. This PR is about retaining the directory location/path when the partition is overwritten.

can you briefly explain what the other commits are ?

nmahadevuni · 2022-03-15T05:00:59Z

The other commits add test infrastructure to run docker based tests using docker java API. The Minio container is used as S3 substitute and HiveHadoop container to run hadoop/hive servers.

highker

@NikhilCollooru, feel free to review the PR. Once you feel it's in a good shape, I can help to take a look.

presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java

presto-hive/src/test/java/com/facebook/presto/hive/BaseTestHiveInsertOverwrite.java

nmahadevuni · 2022-03-21T11:36:25Z

Thanks @NikhilCollooru for the review. I addressed the comments.

NikhilCollooru

LGTM

highker

first 4 commits LGTM; still reviewing

highker · 2022-03-21T20:15:02Z

...o-testing-docker/src/main/java/com/facebook/presto/testing/containers/BaseTestContainer.java

+public abstract class BaseTestContainer
+        implements AutoCloseable
+{
+    private final Logger log;


a static class with direct assignment from Logger.get(BaseTestContainer.getClass());

highker · 2022-03-21T20:15:08Z

...o-testing-docker/src/main/java/com/facebook/presto/testing/containers/BaseTestContainer.java

+    private final Optional<Network> network;
+    private final int startupRetryLimit;
+
+    private GenericContainer<?> container;


highker · 2022-03-21T20:20:15Z

presto-testing-docker/src/main/java/com/facebook/presto/testing/containers/Minio.java

+
+import static java.lang.String.format;
+
+public class Minio


What does minio stand for? I guess it's MinIOContainer?

This is in containers subpackage. Should we rename it?

highker · 2022-03-21T20:21:06Z

presto-hive/src/test/java/com/facebook/presto/hive/containers/HiveHadoop.java

+import java.util.Optional;
+import java.util.Set;
+
+public class HiveHadoop


HiveHadoopContainer?

highker · 2022-03-21T20:22:23Z

presto-hive/src/test/java/com/facebook/presto/hive/containers/HiveMinioDataLake.java

+import static java.util.Objects.requireNonNull;
+import static org.testcontainers.containers.Network.newNetwork;
+
+public class HiveMinioDataLake


Are you asking to change to name to HiveMinIODataLake?

highker · 2022-03-21T20:31:27Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

+import static com.facebook.presto.hive.HiveSessionProperties.InsertExistingPartitionsBehavior;
+import static com.facebook.presto.hive.HiveSessionProperties.InsertExistingPartitionsBehavior.APPEND;
+import static com.facebook.presto.hive.HiveSessionProperties.InsertExistingPartitionsBehavior.ERROR;
+import static com.facebook.presto.hive.HiveSessionProperties.InsertExistingPartitionsBehavior.OVERWRITE;


Let's move InsertExistingPartitionsBehavior to this class so that hive config doesn't depend on hive session properties.

highker · 2022-03-21T21:09:17Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -93,6 +98,7 @@
    private boolean createEmptyBucketFiles = true;
    private boolean insertOverwriteImmutablePartitions;
    private boolean failFastOnInsertIntoImmutablePartitionsEnabled = true;
+    private Optional<InsertExistingPartitionsBehavior> insertExistingPartitionsBehavior = Optional.empty();


it's weird to have optional here. Can we infer the default through getDefaultInsertExistingPartitionsBehavior? Also, I guess our goal here is to deprecate isInsertOverwriteImmutablePartitionEnabled?

isInsertOverwriteImmutablePartitionEnabled is still a valid config. In the HiveSessionProperties, we set the default by calling getInsertExistingPartitionsBehavior() which is same as getDefaultInsertExistingPartitionsBehavior() except getInsertExistingPartitionsBehavior also considers the explicit value if any set by the user for this.

What I meant was once we have insertExistingPartitionsBehavior as an option in the config, there is no need to have isInsertOverwriteImmutablePartitionEnabled anymore. Having the overlapping configs will confuse the users.

I don't think we can infer the default value for this through getDefaultInsertExistingPartitionsBehavior(). If we do, and immutable-partitions is set to true later, we will return APPEND which is not valid.

Then can we make it non optional? Having an optional here makes the config ambiguous; check my other comment on fail hard

highker · 2022-03-21T21:11:10Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSessionProperties.java

-    private static InsertExistingPartitionsBehavior getDefaultInsertExistingPartitionsBehavior(HiveClientConfig hiveClientConfig)
-    {
-        if (!hiveClientConfig.isImmutablePartitions()) {
-            return APPEND;
-        }
-
-        return hiveClientConfig.isInsertOverwriteImmutablePartitionEnabled() ? OVERWRITE : ERROR;
-    }


This function is still valuable with some part of it. Especially when we specify APPEND on immutable partitions (which should throw an error)

In the default case, the method getInsertExistingPartitionsBehavior() is same as this method. We will still throw error for APPEND on immutable partitions. There is no change in that.

highker · 2022-03-21T21:18:25Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -2091,6 +2125,27 @@ else if (partitionUpdate.getUpdateMode() == NEW || partitionUpdate.getUpdateMode
                        .collect(toList())));
    }

+    private void removeNonCurrentQueryFiles(ConnectorSession session, Path partitionPath)


Can we add a javadoc to explain why we need to clean up files here?

highker · 2022-03-21T21:19:45Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

+                    if (handle.getLocationHandle().getWriteMode() == DIRECT_TO_TARGET_EXISTING_DIRECTORY) {
+                        removeNonCurrentQueryFiles(session, partitionUpdate.getTargetPath());
+                    }


Can we also explain a bit more of this branch? We are going to overwrite a partition, but this seems to be deleting the files.

highker · 2022-03-21T21:20:21Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

+                        metastore.addPartition(
+                                session,
+                                handle.getSchemaName(),
+                                handle.getTableName(),
+                                table.getStorage().getLocation(),
+                                false,
+                                partition,
+                                partitionUpdate.getWritePath(),
+                                partitionStatistics);
+                    }
+                }
+                else { // New partition
+                    metastore.addPartition(
+                            session,
+                            handle.getSchemaName(),
+                            handle.getTableName(),
+                            table.getStorage().getLocation(),
+                            false,
+                            partition,
+                            partitionUpdate.getWritePath(),
+                            partitionStatistics);
+                }


It would be good to simplify the logic as they are duplicated code. The old logic is a good example.

nmahadevuni · 2022-03-24T17:20:50Z

Thanks @highker . Addressed all the comments. Please review.

highker · 2022-03-24T18:33:48Z

@nmahadevuni, can you separate out the first 4 commits into a separate PR? Those are good and we can merge faster. For the last one, I might have more comments.

highker · 2022-03-27T00:33:48Z

rebase maybe?

highker

nits only

highker · 2022-03-27T22:45:50Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -53,6 +61,24 @@
        "hive.optimized-reader.enabled"})
 public class HiveClientConfig
 {
+    public enum InsertExistingPartitionsBehavior


can we move it closer to the setter and getter like how other enums are defined in the class?

highker · 2022-03-27T22:47:49Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -93,6 +98,7 @@
    private boolean createEmptyBucketFiles = true;
    private boolean insertOverwriteImmutablePartitions;
    private boolean failFastOnInsertIntoImmutablePartitionsEnabled = true;
+    private Optional<InsertExistingPartitionsBehavior> insertExistingPartitionsBehavior = Optional.empty();


Then can we make it non optional? Having an optional here makes the config ambiguous; check my other comment on fail hard

highker · 2022-03-27T22:48:19Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -604,6 +631,19 @@ public HiveClientConfig setInsertOverwriteImmutablePartitionEnabled(boolean inse
        return this;
    }

+    public InsertExistingPartitionsBehavior getInsertExistingPartitionsBehavior()
+    {
+        return insertExistingPartitionsBehavior.orElse(immutablePartitions ? (isInsertOverwriteImmutablePartitionEnabled() ? OVERWRITE : ERROR) : APPEND);


Let's fail hard if the two configs do not match with each other instead of relying on optional empty

If these configs don't match, we will throw error at line 628 checkArgument call. I have removed optional though.

highker · 2022-03-27T22:48:50Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -604,6 +631,19 @@ public HiveClientConfig setInsertOverwriteImmutablePartitionEnabled(boolean inse
        return this;


Let make isInsertOverwriteImmutablePartitionEnabled a LegacyConfig

To make this LegacyConfig, should property be same as @config? Like below ? Since it is not overridden by any new property.
@LegacyConfig("hive.insert-overwrite-immutable-partitions-enabled")
@config("hive.insert-overwrite-immutable-partitions-enabled")

my bad; actually i meant having a @Deprecated annotation for it.

highker · 2022-03-27T22:53:40Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

-                        partitionStatistics);
+
+                // New partition or overwriting existing partition by staging and moving the new partition
+                if (!existingPartition || (existingPartition && handle.getLocationHandle().getWriteMode() != DIRECT_TO_TARGET_EXISTING_DIRECTORY)) {


This looks like a bug? You mean

if (!existingPartition || partitionUpdate.getUpdateMode() == OVERWRITE)

?

If partitionUpdate.getUpdateMode() != OVERWRITE, then we will throw error on line 2089. If we just check for (partitionUpdate.getUpdateMode() == OVERWRITE) here, even when writeMode is DIRECT_TO_TARGET_EXISTING_DIRECTORY, we will add partition, which is wrong. Since for DIRECT_TO_TARGET_EXISTING_DIRECTORY, the partition already exists and we are removing old files. I simplified the condition to if (!existingPartition || handle.getLocationHandle().getWriteMode() != DIRECT_TO_TARGET_EXISTING_DIRECTORY)

If that is the case, we can still simlify the condition to be

if (!existingPartition || handle.getLocationHandle().getWriteMode() != DIRECT_TO_TARGET_EXISTING_DIRECTORY)

?

highker · 2022-03-28T16:54:59Z

Please fix the test failure; seem to be related

This implementation writes to target partition directory directly. After successful write, all files within partition whose name prefix or suffix don't match current query ID are removed. Cherry-pick of trinodb/trino@96e77e7 Co-authored-by: Arkadiusz Czajkowski <arek@starburstdata.com>

nmahadevuni · 2022-03-29T14:09:56Z

Please fix the test failure; seem to be related

The test failure most likely is because the container stopped before the actual tests ran. This is happening intermittently. I'm now running them in a separate profile in a new job "hive tests / hive-dockerized-tests" in "hive-tests.yml". "test other modules / test-other-modules " failure is not related.

highker · 2022-03-29T15:59:31Z

test-other-modules also failed; can we check?

nmahadevuni · 2022-03-29T16:27:53Z

test-other-modules also failed; can we check?

It timed out. It should have no issues running. Can you please restart just that one?

nmahadevuni · 2022-03-29T17:13:07Z

@highker test-other-modules passed now.

nmahadevuni · 2022-03-31T04:27:48Z

Thank you @NikhilCollooru and @highker for the review.

nmahadevuni changed the title ~~Hive insert overwrite when using S3 storage~~ WIP: Hive insert overwrite when using S3 storage Feb 16, 2022

nmahadevuni force-pushed the s3overwritepartition branch 4 times, most recently from 2e5a6aa to be04a09 Compare February 18, 2022 11:06

nmahadevuni changed the title ~~WIP: Hive insert overwrite when using S3 storage~~ Hive insert overwrite when using S3 storage Feb 18, 2022

jainxrohit requested review from jainxrohit, tdcmeehan and highker February 18, 2022 18:58

highker reviewed Mar 18, 2022

View reviewed changes

NikhilCollooru reviewed Mar 18, 2022

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java Show resolved Hide resolved

presto-hive/src/test/java/com/facebook/presto/hive/BaseTestHiveInsertOverwrite.java Show resolved Hide resolved

nmahadevuni force-pushed the s3overwritepartition branch 3 times, most recently from 5e8ccf9 to 99a028b Compare March 21, 2022 10:08

NikhilCollooru approved these changes Mar 21, 2022

View reviewed changes

NikhilCollooru requested a review from highker March 21, 2022 15:49

highker reviewed Mar 21, 2022

View reviewed changes

nmahadevuni force-pushed the s3overwritepartition branch from 99a028b to dce9a5e Compare March 24, 2022 11:55

highker self-requested a review March 24, 2022 18:17

nmahadevuni mentioned this pull request Mar 25, 2022

Add dockerized Hive with MinIO test containers for data lake tests #17520

Merged

nmahadevuni force-pushed the s3overwritepartition branch from dce9a5e to dd5e9ab Compare March 27, 2022 18:23

highker approved these changes Mar 27, 2022

View reviewed changes

highker self-assigned this Mar 27, 2022

nmahadevuni force-pushed the s3overwritepartition branch 3 times, most recently from 32c890c to d269b17 Compare March 29, 2022 10:23

nmahadevuni force-pushed the s3overwritepartition branch from d269b17 to 7860880 Compare March 29, 2022 11:46

highker merged commit 3fbbb67 into prestodb:master Mar 31, 2022

nmahadevuni deleted the s3overwritepartition branch March 31, 2022 04:24

mshang816 mentioned this pull request May 17, 2022

Add release notes for 0.273 #17775

Merged

14 tasks

		@@ -604,6 +631,19 @@ public HiveClientConfig setInsertOverwriteImmutablePartitionEnabled(boolean inse
		return this;

Hive insert overwrite when using S3 storage #17307

Hive insert overwrite when using S3 storage #17307

Conversation

nmahadevuni commented Feb 16, 2022 • edited by highker Loading

nmahadevuni commented Feb 18, 2022

NikhilCollooru commented Mar 14, 2022 • edited Loading

nmahadevuni commented Mar 14, 2022

NikhilCollooru commented Mar 14, 2022

nmahadevuni commented Mar 15, 2022

highker left a comment

Choose a reason for hiding this comment

nmahadevuni commented Mar 21, 2022

NikhilCollooru left a comment

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmahadevuni Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmahadevuni commented Mar 24, 2022

highker commented Mar 24, 2022

highker commented Mar 27, 2022

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

highker commented Mar 28, 2022

nmahadevuni commented Mar 29, 2022 • edited Loading

highker commented Mar 29, 2022

nmahadevuni commented Mar 29, 2022

nmahadevuni commented Mar 29, 2022

nmahadevuni commented Mar 31, 2022

nmahadevuni commented Feb 16, 2022 •

edited by highker

Loading

NikhilCollooru commented Mar 14, 2022 •

edited

Loading

nmahadevuni Mar 23, 2022 •

edited

Loading

nmahadevuni commented Mar 29, 2022 •

edited

Loading