Add Presto iceberg connector #15836

ChunxuTang · 2021-03-15T21:28:55Z

Cherry-pick of trinodb/trino@e82c2d5

Co-Authored-By: Parth Brahmbhatt pbrahmbhatt@netflix.com
Co-Authored-By: Zhenxiao Luo zluo@twitter.com
Co-Authored-By: Beinan Wang beinanw@twitter.com

== RELEASE NOTES ==

General Changes
* Add iceberg connector

beinan · 2021-03-15T22:05:53Z

Hello @ChunxuTang could you rebase master? Thanks!

ChunxuTang · 2021-03-15T22:07:27Z

#15620

ChunxuTang · 2021-03-16T01:27:37Z

After the rebase, it seems that some modules won't be compiled successfully. Will fix that.

ChunxuTang · 2021-03-17T03:01:06Z

Provided a quick fix for the parquet conflicts to kick-off the review and testing. Also sent a PR to the prestodb/presto-hive-apache repo for a parquet upgrade. When that PR is merged, we can remove the quick fix safely.

beinan

Great work! @ChunxuTang Just put a few questions and code style issues

beinan · 2021-03-23T18:46:59Z

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

@@ -1330,7 +1330,7 @@ private static Table buildTableObject(
        return tableBuilder.build();
    }

-    private static PrincipalPrivileges buildInitialPrivilegeSet(String tableOwner)
+    public static PrincipalPrivileges buildInitialPrivilegeSet(String tableOwner)


Why do we need to make it public? any usage of this method outside?

Good catch! Have changed it back to private.

beinan · 2021-03-23T19:02:17Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HdfsFileIo.java

+import static com.facebook.presto.iceberg.IcebergErrorCode.ICEBERG_FILESYSTEM_ERROR;
+import static java.util.Objects.requireNonNull;
+
+public class HdfsFileIo


Io or IO? I think using upper case 'O' looks better

Yes, using the upper case of 'O' is better here. Refactored the code here.

beinan · 2021-03-23T19:09:11Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

+
+    private HiveTableOperations(FileIO fileIo, ExtendedHiveMetastore metastore, String database, String table, Optional<String> owner, Optional<String> location)
+    {
+        this.fileIo = requireNonNull(fileIo, "fileIo is null");


Also updated~

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergModule.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergSessionProperties.java

beinan · 2021-03-23T21:48:44Z

presto-spark-base/pom.xml

@@ -245,6 +245,10 @@
                        <ignoredResourcePattern>parquet.thrift</ignoredResourcePattern>
                        <ignoredResourcePattern>about.html</ignoredResourcePattern>
                    </ignoredResourcePatterns>
+                    <ignoredClassPatterns>


is this change still needed after we update presto-hive-apache? if not, add a todo or something?

Nice catch! Yeah, I think we may still need this after updating the parquet in the presto-hive-apache to avoid dependency conflicts. We can do more tests of compilation to confirm that, avoiding potential changes in other packages' POM files.

beinan · 2021-03-23T21:51:50Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergDistributed.java

+
+    @Override
+    public void testRenameTable()
+    {


leave a comment if the rename table feature has not been supported yet, or add a todo if it's already supported

Yeah, we don't support it yet. The reason we have the function is that we need to override the function defined in the AbstractTestDistributedQueries. We leverage that class to test SQL queries against the connector.

zhenxiao

nice work, @ChunxuTang
added a few comments. could you please add cherry-pick info, and original commit in commit message?

zhenxiao · 2021-04-25T03:47:45Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

+        PrestoPrincipal owner = new PrestoPrincipal(USER, table.getOwner());
+        PrincipalPrivileges privileges = new PrincipalPrivileges(
+                ImmutableMultimap.<String, HivePrivilegeInfo>builder()
+                        .put(table.getOwner(), new HivePrivilegeInfo(HivePrivilege.SELECT, true, owner, owner))


static import HivePrivilege.SELECT
same for INSERT, UPDATE, DELETE

Sure, updated in the new commit.

zhenxiao · 2021-04-25T03:48:01Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

+            try {
+                io().deleteFile(newMetadataLocation);
+            }
+            catch (RuntimeException ex) {


s/ex/exception/g

Name updated. Thanks for figuring it out!

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

zhenxiao · 2021-04-25T03:52:44Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergMetadata.java

+                // table disappeared during listing operation
+            }
+            catch (UnknownTableTypeException e) {
+                // ignore table of unknown type


shall we throw exception or log warning here? ignore silently seems not right

Nice catch! Added extra logging here.

zhenxiao · 2021-04-25T03:52:58Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergMetadata.java

+                columns.put(table, getTableMetadata(session, table).getColumns());
+            }
+            catch (TableNotFoundException e) {
+                // table disappeared during listing operation


shall we throw exception or log warning here?

Same as above. Added extra logging.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSource.java

zhenxiao · 2021-04-25T03:56:55Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableName.java

+        }
+
+        Optional<Long> version = Optional.empty();
+        if (type == TableType.DATA || type == TableType.PARTITIONS || type == TableType.MANIFESTS || type == TableType.FILES) {


static import DATA, PARTITIONS, MANIFESTS, FILES

Sure, updated~

zhenxiao · 2021-04-25T03:57:24Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableName.java

+
+        String table = match.group("table");
+        String typeString = match.group("type");
+        String ver1 = match.group("ver1");


s/ver1/version1/g
s/ver2/version2/g

Sure, updated the variable names.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/HiveTableOperations.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergModule.java

ChunxuTang · 2021-05-07T22:20:02Z

nice work, @ChunxuTang
added a few comments. could you please add cherry-pick info, and original commit in commit message?

Sure~ Added the cherry-pick info in both the PR and the commit message.

beinan · 2021-05-11T02:33:30Z

Hello @rongrong , looks like the Facebook Integration test keep failing for this PR.

Facebook Integration — Build finished. No test results found.

is it possible to help us check what is happening to the test? Thanks a lot!

zhenxiao

looks good to me
thank you @ChunxuTang
a few minor issues

zhenxiao · 2021-05-14T11:52:48Z

presto-iceberg/pom.xml

+    <parent>
+        <groupId>com.facebook.presto</groupId>
+        <artifactId>presto-root</artifactId>
+        <version>0.253-SNAPSHOT</version>


update to latest version, 254-snapshot

Sure~ Rebased the master branch and upgraded the presto-iceberg module to 0.254-snapshot.

zhenxiao · 2021-05-14T12:01:59Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergErrorCode.java

+    ICEBERG_CURSOR_ERROR(9, EXTERNAL),
+    ICEBERG_WRITE_VALIDATION_FAILED(10, INTERNAL_ERROR),
+    ICEBERG_INVALID_SNAPSHOT_ID(11, USER_ERROR),
+    /**/;


remove /**/

Thanks for the suggestion! Removed the unnecessary comment.

Cherry-pick of trinodb/trino@e82c2d5 Co-Authored-By: Parth Brahmbhatt <pbrahmbhatt@netflix.com> Co-Authored-By: Zhenxiao Luo <zluo@twitter.com> Co-Authored-By: Beinan Wang <beinanw@twitter.com>

zhenxiao · 2021-05-18T12:06:49Z

hi @tdcmeehan seems all tests passed, except for Facebook integration. could you please help check if this PR is good to proceed?

tdcmeehan · 2021-05-19T01:06:46Z

The Presto Facebook test failure is unrelated.

ChunxuTang force-pushed the presto-iceberg branch from a4fea0d to d68497d Compare March 15, 2021 22:40

ChunxuTang mentioned this pull request Mar 16, 2021

Upgrade parquet to 1.11.0 prestodb/presto-hive-apache#46

Merged

beinan reviewed Mar 23, 2021

View reviewed changes

ChunxuTang force-pushed the presto-iceberg branch from 4d30413 to d6acd7d Compare April 21, 2021 00:47

zhenxiao requested changes Apr 25, 2021

View reviewed changes

rohanpednekar added the Roadmap A top level roadmap item label May 5, 2021

beinan approved these changes May 6, 2021

View reviewed changes

ChunxuTang force-pushed the presto-iceberg branch 2 times, most recently from a479836 to 1a4c150 Compare May 7, 2021 21:10

ChunxuTang force-pushed the presto-iceberg branch from 8d0b364 to 54a5a75 Compare May 8, 2021 05:29

zhenxiao approved these changes May 14, 2021

View reviewed changes

ChunxuTang and others added 13 commits May 14, 2021 10:56

upgrade parquet to 1.11.0

064c724

Add iceberg connector

d09a67d

Cherry-pick of trinodb/trino@e82c2d5 Co-Authored-By: Parth Brahmbhatt <pbrahmbhatt@netflix.com> Co-Authored-By: Zhenxiao Luo <zluo@twitter.com> Co-Authored-By: Beinan Wang <beinanw@twitter.com>

Set up query tests

47d002c

Include iceberg connector in the root POM

573c62e

Fix parquet version conflict

7aa5740

Refactor HdfsFileIo names

d480372

Upgrade Presto to 0.252

fcc9058

Fix review comments

f78301b

Adjust the usage of Hive Metastore with MetastoreContext

839c81a

Add MetastoreContext in the iceberg HiveTableOperations

209fbe6

Remove unnecessary comments

6d6f53e

Include presto-iceberg in example config

a99ca25

Adjust the test change of AbstractTestDistributedQueries

f1c2088

Upgrade to 0.254

a55462e

ChunxuTang force-pushed the presto-iceberg branch from 54a5a75 to a55462e Compare May 14, 2021 18:26

beinan merged commit b8b0e52 into prestodb:master May 20, 2021

NikhilCollooru mentioned this pull request May 20, 2021

Fix MetastoreContext creation in presto-iceberg module #16130

Merged

sujay-jain mentioned this pull request May 21, 2021

Add release notes for 0.254 #16141

Merged

10 tasks

ajaygeorge mentioned this pull request May 26, 2021

[TEST] Add release notes for 0.254 #16165

Closed

16 tasks

Add Presto iceberg connector #15836

Add Presto iceberg connector #15836

Conversation

ChunxuTang commented Mar 15, 2021 • edited

beinan commented Mar 15, 2021

ChunxuTang commented Mar 15, 2021

ChunxuTang commented Mar 16, 2021

ChunxuTang commented Mar 17, 2021

beinan 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

zhenxiao 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChunxuTang commented May 7, 2021

beinan commented May 11, 2021

zhenxiao 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

zhenxiao commented May 18, 2021

tdcmeehan commented May 19, 2021

ChunxuTang commented Mar 15, 2021 •

edited