Read support for Hive's INSERT ONLY ACID tables #1034

shubhamtagra · 2019-06-24T12:03:43Z

Implementation of https://docs.google.com/document/d/1GwSNc_6jSP8N1SyJIHnaMeWvh7JzhGPZ7QKm1DHJqEw

Relates to #576

presto-product-tests/conf/docker/acid/docker-compose.yml

electrum · 2019-06-24T14:49:40Z

I quickly skimmed this. Looks fairly straightforward. Thanks a lot for adding this feature.

Can you pull out the changes to HiveMetastore and ThriftMetastore to be in a separate commit? That will keep the main commit smaller and easier to review. Something like

Add transactional support to metastore interface

shubhamtagra · 2019-07-09T06:35:50Z

I quickly skimmed this. Looks fairly straightforward. Thanks a lot for adding this feature.

Can you pull out the changes to HiveMetastore and ThriftMetastore to be in a separate commit? That will keep the main commit smaller and easier to review. Something like

Add transactional support to metastore interface

This is done

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java

findepi

Just skimming.

In particular, I noticed there will be a bit of naming comments, as certain names do not align with our code style (abbreviations, or spelling of well-known acronyms in names, or spelling of "heartbeat"). I have marked some of them, but I think it could be more effective, if you could take another look at the code and try to update the names. Then the review will be more focused on merits rather than naming.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSessionProperties.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveErrorCode.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSessionProperties.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveWriteUtils.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveACIDState.java

findepi

(#1034 (review))

findepi · 2019-08-02T09:31:05Z

presto-product-tests/conf/docker/acid/compose.sh

@@ -0,0 +1,10 @@
+#!/usr/bin/env bash


When is this environment run?

Anyway, #1239 will make test scaffolding sane here.

This had to added to scripts, I was running it by hand but I see with your change it will be default. Will wait for #1239 to merge and then remove this env.

findepi · 2019-09-16T09:13:28Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

@@ -379,13 +389,42 @@ private void invokeNoMoreSplitsIfNecessary()

        // Bucketed partitions are fully loaded immediately since all files must be loaded to determine the file to bucket mapping
        if (tableBucketInfo.isPresent()) {
+            if (AcidUtils.isTransactionalTable(table.getParameters())) {
+                throw new PrestoException(NOT_SUPPORTED, "Presto cannot read bucketted hive transactional tables");


This seems to be a serious limitation. For Hive < 3, a transactional table has to be bucketed.

Yes, this is one of the next items to be picked.

presto-main/src/main/java/io/prestosql/execution/SqlQueryExecution.java

presto-main/src/main/java/io/prestosql/metadata/InsertTableHandle.java

presto-main/src/main/java/io/prestosql/sql/planner/TableHandleExtractor.java

...-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorTableHandle.java

presto-product-tests/src/main/java/io/prestosql/tests/TestGroups.java

...uct-tests/src/main/java/io/prestosql/tests/hive/acid/TestPartitionedInsertOnlyAcidTable.java

presto-product-tests/pom.xml

...uct-tests/src/main/java/io/prestosql/tests/hive/acid/TestPartitionedInsertOnlyAcidTable.java

presto-product-tests/conf/tempto/tempto-configuration-for-docker-acid.yaml

presto-product-tests/conf/docker/acid/docker-compose.yml

...uct-tests/src/main/java/io/prestosql/tests/hive/acid/TestPartitionedInsertOnlyAcidTable.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreClient.java

presto-main/src/main/java/io/prestosql/metadata/InsertTableHandle.java

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorTableHandle.java

electrum

I need to do another pass over some parts in more detail, but overall this looks great.

presto-product-tests/conf/docker/acid/docker-compose.yml

...to-product-tests/src/main/java/io/prestosql/tests/hive/acid/TestInsertOnlyAcidTableRead.java

presto-product-tests/src/main/java/io/prestosql/tests/hive/acid/TestFullAcidTableRead.java

shubhamtagra · 2019-11-20T14:47:23Z

Handled your comments @findepi

findepi

I bunch of comments. Overall looks great.
Few outstanding questions.

To make the process more efficient, I made some fixups.
Please double check whcih comments i managed to fixup which i didn't -- and whether you agree with the fixups.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java

...to-hive/src/main/java/io/prestosql/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java

presto-main/src/main/java/io/prestosql/execution/SqlQueryExecution.java

...-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java

presto-hive/src/test/java/io/prestosql/plugin/hive/AbstractTestHive.java

presto-hive/src/test/java/io/prestosql/plugin/hive/AbstractTestHiveFileSystem.java

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java

...to-product-tests/src/main/java/io/prestosql/tests/hive/acid/TestInsertOnlyAcidTableRead.java

findepi · 2019-11-22T14:38:16Z

@stagraqubole the PR needs a rebase.

Please do the following

squash the fixups if you agree with them
rebase changing as little as possible
apply outstanding changes as fixups

thanks for your work on this!

dain

Let's merge this after the current release goes out.

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java

findepi

LGTM % nits.

Requesting changes or discussion around handling of original files.
I'd prefer to split this out into separate PR.

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

...to-hive/src/main/java/io/prestosql/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

findepi

Thanks, please squash.

findepi · 2019-11-28T21:54:13Z

Merged, thanks!

testBad() fails on TestHiveInMemoryMetastore with: java.lang.IllegalStateException: Query already begun: Optional[20200306_080241_00002_adgin] while starting query 20200306_080241_00003_adgin at com.google.common.base.Preconditions.checkState(Preconditions.java:823) at io.prestosql.plugin.hive.metastore.SemiTransactionalHiveMetastore.beginQuery(SemiTransactionalHiveMetastore.java:999) at io.prestosql.plugin.hive.HiveMetadata.beginQuery(HiveMetadata.java:2367) at io.prestosql.plugin.hive.AbstractTestHive.testBad(AbstractTestHive.java:954)

cla-bot bot added the cla-signed label Jun 24, 2019

shubhamtagra commented Jun 24, 2019

View reviewed changes

presto-product-tests/conf/docker/acid/docker-compose.yml Outdated Show resolved Hide resolved

shubhamtagra mentioned this pull request Jun 26, 2019

Add image for hive3 and hadoop3 trinodb/docker-images#13

Closed

shubhamtagra force-pushed the acid-ii branch from be5cad2 to f211af9 Compare July 9, 2019 06:33

kokosing reviewed Jul 25, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java Show resolved Hide resolved

findepi reviewed Jul 26, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java Outdated Show resolved Hide resolved

findepi reviewed Jul 26, 2019

View reviewed changes

findepi previously requested changes Aug 1, 2019

View reviewed changes

findepi mentioned this pull request Aug 2, 2019

Use HDP 3.1 by default in product tests #1239

Closed

findepi reviewed Aug 2, 2019

View reviewed changes

shubhamtagra force-pushed the acid-ii branch 2 times, most recently from bb7398a to 6cbf287 Compare August 5, 2019 16:06

shubhamtagra mentioned this pull request Aug 6, 2019

Support for reading full ACID Hive Tables #1257

Closed

shubhamtagra force-pushed the acid-ii branch 4 times, most recently from 9e954de to aeb1e90 Compare August 8, 2019 16:04

findepi mentioned this pull request Aug 20, 2019

I connected hive 3.1 times incorrectly #1341

Closed

findepi reviewed Sep 16, 2019

View reviewed changes

...uct-tests/src/main/java/io/prestosql/tests/hive/acid/TestPartitionedInsertOnlyAcidTable.java Outdated Show resolved Hide resolved

findepi reviewed Sep 16, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreClient.java Outdated Show resolved Hide resolved

shubhamtagra force-pushed the acid-ii branch from aeb1e90 to 094d897 Compare September 25, 2019 15:48

electrum reviewed Sep 26, 2019

View reviewed changes

shubhamtagra force-pushed the acid-ii branch 2 times, most recently from 0d1bdcc to d76ee37 Compare September 30, 2019 08:13

shubhamtagra force-pushed the acid-ii branch from d76ee37 to c58cff6 Compare October 4, 2019 05:10

findepi reviewed Nov 8, 2019

View reviewed changes

shubhamtagra force-pushed the acid-ii branch from bc80a71 to e43b261 Compare November 20, 2019 14:45

shubhamtagra force-pushed the acid-ii branch 4 times, most recently from 0decba8 to 22e5208 Compare November 21, 2019 09:49

dain mentioned this pull request Nov 21, 2019

[WIP] Query cleanup before auto commit #2067

Closed

findepi assigned findepi and unassigned dain Nov 22, 2019

findepi reviewed Nov 22, 2019

View reviewed changes

dain mentioned this pull request Nov 23, 2019

Read Hive ORC ACID tables #2068

Merged

shubhamtagra force-pushed the acid-ii branch 2 times, most recently from bd49dba to 024a743 Compare November 25, 2019 16:07

dain approved these changes Nov 25, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Show resolved Hide resolved

findepi requested changes Nov 26, 2019

View reviewed changes

shubhamtagra force-pushed the acid-ii branch from 024a743 to a601f1a Compare November 27, 2019 14:43

findepi approved these changes Nov 27, 2019

View reviewed changes

dain and others added 3 commits November 28, 2019 13:15

Call begin query when a catalog metadata is accessed

942bff5

Handle exceptions from Metadata.cleanupQuery

afeac85

Read support for Hive's INSERT ONLY Acid tables

aad26e8

shubhamtagra force-pushed the acid-ii branch from f314b8f to aad26e8 Compare November 28, 2019 07:45

findepi mentioned this pull request Nov 28, 2019

[for CI] Read support for Hive's INSERT ONLY ACID tables #2143

Merged

findepi added this to the 327 milestone Nov 28, 2019

findepi closed this Nov 28, 2019

findepi mentioned this pull request Nov 28, 2019

Release notes for 327 #2124

Closed

7 tasks

findepi mentioned this pull request Dec 17, 2019

Support reading ACID/Transactional tables with "original files" #2293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read support for Hive's INSERT ONLY ACID tables #1034

Read support for Hive's INSERT ONLY ACID tables #1034

shubhamtagra commented Jun 24, 2019 •

edited by findepi

Loading

electrum commented Jun 24, 2019

shubhamtagra commented Jul 9, 2019

findepi left a comment

findepi left a comment

findepi Aug 2, 2019

shubhamtagra Aug 2, 2019

findepi Sep 16, 2019

shubhamtagra Sep 24, 2019

electrum left a comment

shubhamtagra commented Nov 20, 2019

findepi left a comment

findepi commented Nov 22, 2019 •

edited

Loading

dain left a comment

findepi left a comment

findepi left a comment

findepi commented Nov 28, 2019

Read support for Hive's INSERT ONLY ACID tables #1034

Read support for Hive's INSERT ONLY ACID tables #1034

Conversation

shubhamtagra commented Jun 24, 2019 • edited by findepi Loading

electrum commented Jun 24, 2019

shubhamtagra commented Jul 9, 2019

findepi left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi Aug 2, 2019

Choose a reason for hiding this comment

shubhamtagra Aug 2, 2019

Choose a reason for hiding this comment

findepi Sep 16, 2019

Choose a reason for hiding this comment

shubhamtagra Sep 24, 2019

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

shubhamtagra commented Nov 20, 2019

findepi left a comment

Choose a reason for hiding this comment

findepi commented Nov 22, 2019 • edited Loading

dain left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi commented Nov 28, 2019

shubhamtagra commented Jun 24, 2019 •

edited by findepi

Loading

findepi commented Nov 22, 2019 •

edited

Loading