Add hudi connector #17149

7c00 · 2021-12-31T09:02:39Z

Add hudi connector

Test plan - Unit tests.

== RELEASE NOTES ==

Connectors
* Introduce Hudi connector

codope · 2022-01-20T11:54:54Z

cc @yihua @bhasudha @garyli1019 @agrawaldevesh

rohanpednekar · 2022-01-21T02:36:12Z

CC @pratyakshsharma @imjalpreet

7c00 · 2022-01-27T02:57:03Z

Thanks @codope @rohanpednekar!
Hello @pratyakshsharma @imjalpreet @yihua @bhasudha @garyli1019 @agrawaldevesh, could you please help review this PR? Thanks in advance.

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/PartitionMetadata.java

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiMetadata.java

codope

@7c00 Thanks for the contribution. This would be pretty useful for the Hudi community.

I took one pass and left some comments. I have a high-level question. Can you please add to the description, what all Hudi table types and query types are currently supported?

pom.xml

presto-hive-common/src/main/java/org/apache/hadoop/fs/HadoopExtendedFileSystem.java

presto-hudi/pom.xml

presto-hudi/src/main/java/com/facebook/presto/hive/HudiRecordCursors.java

presto-hudi/src/main/java/com/facebook/presto/hudi/ParquetPageSourceUtil.java

presto-hudi/src/main/java/com/facebook/presto/hudi/RecordCursorUtil.java

codope · 2022-01-31T16:24:16Z

presto-hudi/src/main/java/com/facebook/presto/hudi/RecordCursorUtil.java

+    private static InputFormat<?, ?> createInputFormat(Configuration conf, String inputFormat)
+    {
+        try {
+            Class<?> clazz = conf.getClassByName(inputFormat);


Instead of reflection-based check, we can use annotations provided by Hudi. Check these two annotations in Hudi:

UseRecordReaderFromInputFormat UseFileSplitsFromInputFormat

The annotations UseRecordReaderFromInputFormat and UseFileSplitsFromInputFormat indicate that we could delegate the splitting to InputFormat. As we discussed in the design proposal, there are some performance issues in the splitting implementation from InputFormat. So we will ignore these annotations and provide a new splitting implementation optimized for Presto.

codope · 2022-01-31T16:28:33Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiSplitManager.java

+                    format("Invalid partition name %s for partition columns %s", partitionName, partitionColumns));
+            Partition partition = metastore.getPartition(context, databaseName, tableName, partitionValues)
+                    .orElseThrow(() -> new PrestoException(HUDI_INVALID_METADATA, format("Partition %s expected but not found", partitionName)));
+            Map<String, String> keyValues = zipPartitionKeyValues(partitionColumns, partitionValues);


Not sure if you're considering different partition extractors in hudi-hive sync. Depending on partition extractor, partition values can change.

We are now retrieving partition metadata through HiveMetaStore, where the partition values stored have been extracted by some partition extractors from their partition locations and thus we dont need to take consisder of the partition extractors.

7c00 · 2022-02-14T13:00:33Z

@pratyakshsharma @codope Thanks for your comments. I have updated the code as the comments. Could you please take a second review? Thanks in advance.

pratyakshsharma · 2022-02-20T12:54:23Z

@codope Can you please take another pass on this?

codope · 2022-02-21T05:46:06Z

@pratyakshsharma @codope Thanks for your comments. I have updated the code as the comments. Could you please take a second review? Thanks in advance.

Will review by Wednesday.

7c00 · 2022-02-25T06:51:25Z

ping @codope

codope · 2022-02-25T12:09:05Z

@7c00 I have taken one more pass. The code look fine from Hudi perspective.
@arunthirupathi Could you or someone in the community help in taking this forward?
cc @agrawaldevesh

arunthirupathi · 2022-02-25T19:06:59Z

This contains 2 set of changes, changes to hive/metastore and hudi connector. Can you please do a PR for hive/metastore changes alone and hudi connector.

I will review the hive/metastore changes closely.
Hudi, if you folks are happy with it, I will glance through it and help you in merging.

7c00 · 2022-02-28T09:43:58Z

@arunthirupathi I have extracted the metastore related changes to a new PR at #17368. Could you take a review on it? Thank you in advance ❤️ .

7c00 · 2022-03-10T14:17:17Z

I have refactored the tests. Instead of dynamically generating tables during testing, the new tests are using the pre-generated data from Hudi Docker demo. The reasons for that are

it's hard to make PrestoExtendedFileSystemCache happy with HoodieWrapperFileSystem; the issue was walked around in a very tricky way;
HoodieJavaClient cannot create a MOR table with log files, which makes the tests imcomplete

@codope @pratyakshsharma Could you take a second review? Nearly all the changes comes from the test part. Then I'd like to invite Presto fellows to review the PR.

pratyakshsharma · 2022-03-10T15:28:24Z

I am actually travelling this weekend. Will take a look early next week.

arunthirupathi · 2022-03-10T17:28:18Z

What is the size of the jar that is getting built ?few 10's of MB is fine, but if it is producing really large then please look into it in reducing the size.

When it is ready for review, Please ping me again.

For specific connectors, we require it to be thoroughly reviewed by someone else familiar with the connector. Once that review is done, I will make a pass to ensure it meets the coding guidelines.

7c00 · 2022-04-11T07:14:40Z

@arunthirupathi Sorry for late to reply.

What is the size of the jar that is getting built ?few 10's of MB is fine, but if it is producing really large then please look into it in reducing the size.

As we are reusing dependencies from presto-hive module and have added no new dependency. The total size of this hudi connector will be less than that of hive connector.

When it is ready for review, Please ping me again.

For specific connectors, we require it to be thoroughly reviewed by someone else familiar with the connector. Once that review is done, I will make a pass to ensure it meets the coding guidelines.

Now this PR is blocked the Avro dependency issue (found in #17463) to be fixed in next Hudi release 0.11. When Hudi 0.11 is released, we are going to continue to work on this PR and invite you to review. Thanks you in advance.

cc @codope

codope · 2022-05-02T08:14:48Z

Now this PR is blocked the Avro dependency issue (found in #17463) to be fixed in next Hudi release 0.11. When Hudi 0.11 is released, we are going to continue to work on this PR and invite you to review. Thanks you in advance.

@7c00 Hudi v0.11 is released https://mvnrepository.com/artifact/org.apache.hudi/hudi-presto-bundle/0.11.0
Can you please upgrade and rebase this PR?

7c00 · 2022-05-25T10:05:15Z

The code has been rebased to sync with master branch after #17463 . It's ready to review now.

@codope @pratyakshsharma @arunthirupathi @kewang1024 @highker Could you take a look at this PR? Thanks in advance! ❤️

pratyakshsharma · 2022-06-07T06:16:12Z

@pettyjamesm Please take a look when you get a chance.

arunthirupathi

Mostly looks good, left some nits.

Sorry my notification in Presto is messed up, I will fix it. so that I will have a quicker turn around time next time.

arunthirupathi · 2022-06-10T23:58:17Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiColumnHandle.java

+    @Override
+    public String toString()
+    {
+        return id + ":" + name + ":" + hiveType + ":" + columnType;


Can you please StringHelper to convert to String ?

The toString method here is implemented followed the convention of HiveColumnHandle#toString. In this way, we could have a more compacted text (refer to a Hudi column) when output logs.

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiConnectorFactory.java

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiFile.java

arunthirupathi · 2022-06-11T00:03:55Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiFile.java

+    @Override
+    public String toString()
+    {
+        return path + ":" + start + "+" + length;


Use toStringHelper

The same reason as HudiColumnHandle#toString

arunthirupathi · 2022-06-11T00:13:09Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiPageSource.java

+            int batchSize = dataPage.getPositionCount();
+            Block[] blocks = new Block[prefilledBlocks.length];
+            for (int i = 0; i < prefilledBlocks.length; i++) {
+                if (prefilledBlocks[i] != null) {
+                    blocks[i] = new RunLengthEncodedBlock(prefilledBlocks[i], batchSize);
+                }
+                else {
+                    blocks[i] = dataPage.getBlock(delegateIndexes[i]);
+                }
+            }
+            return new Page(batchSize, blocks);


This code though is right, is inefficient. This is one of the hot paths. If there is no prefilled blocks, short circuit and return the delegate page.

Good catch, thank you. Add short circuit when no actual prefilled block.

arunthirupathi · 2022-06-11T00:13:55Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiPageSource.java

+                if (valueString.equalsIgnoreCase("false")) {
+                    return false;
+                }
+                throw new IllegalArgumentException();


Add the value here on what was illegal, so that it makes debugging easier.

The IllegalArgumentException is catched later in current method, then a PrestoException with more context infomation is thrown.

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiPageSource.java

arunthirupathi · 2022-06-11T00:15:37Z

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiPageSource.java

+                BigDecimal decimal = new BigDecimal(valueString);
+                decimal = decimal.setScale(decimalType.getScale(), BigDecimal.ROUND_UNNECESSARY);
+                if (decimal.precision() > decimalType.getPrecision()) {
+                    throw new IllegalArgumentException();


Include the decimalPrecision and decimalType precision in the error message, along with the valueString.

The same reaseon as above.

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiPlugin.java

codope · 2022-06-13T04:11:42Z

Thanks for reviewing @arunthirupathi
We'll address the comments shortly.

arunthirupathi · 2022-06-13T05:38:58Z

Looks like there were some changes and this was broken, can you please fix them as well ?

7c00 · 2022-06-13T06:02:46Z

@arunthirupathi Thanks for your reviewing. The comments have been addressed. Let me ping you again when all CI checks passed.

7c00 · 2022-06-13T07:06:29Z

Hey @arunthirupathi All CI passed. Let's merge this PR?

7c00 · 2022-06-14T01:41:45Z

Thanks @arunthirupathi @codope @pratyakshsharma @rohanpednekar for all your time and help! ❤️

7c00 force-pushed the presto-hudi branch from a135f1d to f6764c4 Compare January 11, 2022 13:12

rongrong marked this pull request as ready for review January 13, 2022 03:23

7c00 force-pushed the presto-hudi branch 3 times, most recently from dc1acb2 to 75b1762 Compare January 17, 2022 03:12

7c00 force-pushed the presto-hudi branch 2 times, most recently from fbb24fd to df82b6d Compare January 27, 2022 02:47

pratyakshsharma reviewed Jan 27, 2022

View reviewed changes

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/PartitionMetadata.java Outdated Show resolved Hide resolved

pratyakshsharma reviewed Jan 27, 2022

View reviewed changes

presto-hudi/src/main/java/com/facebook/presto/hudi/HudiMetadata.java Outdated Show resolved Hide resolved

7c00 force-pushed the presto-hudi branch 2 times, most recently from d3c130c to 06ab601 Compare January 29, 2022 07:55

codope reviewed Jan 31, 2022

View reviewed changes

7c00 force-pushed the presto-hudi branch 2 times, most recently from 83edce7 to 23c5fd9 Compare February 14, 2022 12:22

7c00 requested a review from pratyakshsharma February 14, 2022 13:00

7c00 mentioned this pull request Feb 28, 2022

Generalize storage formats in file metastore #17368

Merged

7c00 force-pushed the presto-hudi branch 2 times, most recently from aa78a34 to db07aa6 Compare March 10, 2022 13:58

7c00 mentioned this pull request Mar 16, 2022

Add hudi integeration tests and bump version #17463

Merged

codope mentioned this pull request May 10, 2022

[FEATURE REQUEST] Support for Presto/Athena to make updates to Hudi Tables apache/hudi#5021

Closed

7c00 force-pushed the presto-hudi branch from db07aa6 to 2d345c5 Compare May 25, 2022 08:12

7c00 requested a review from a team as a code owner May 25, 2022 08:12

7c00 requested a review from presto-oss May 25, 2022 08:12

7c00 force-pushed the presto-hudi branch from 2d345c5 to c0170cb Compare May 25, 2022 08:15

arunthirupathi approved these changes Jun 11, 2022

View reviewed changes

7c00 force-pushed the presto-hudi branch from c0170cb to 5ada852 Compare June 13, 2022 05:04

7c00 force-pushed the presto-hudi branch from 5ada852 to 988f119 Compare June 13, 2022 05:56

Add hudi connector

ad99b5a

7c00 force-pushed the presto-hudi branch from 988f119 to ad99b5a Compare June 13, 2022 05:59

arunthirupathi merged commit 6ecfd81 into prestodb:master Jun 13, 2022

miomiocat mentioned this pull request Jun 22, 2022

[Design] Separate connector module for Hudi #17006

Closed

highker mentioned this pull request Jul 6, 2022

Add release notes for 0.274 #17987

Closed

7 tasks

Add hudi connector #17149

Add hudi connector #17149

Conversation

7c00 commented Dec 31, 2021 • edited Loading

codope commented Jan 20, 2022

rohanpednekar commented Jan 21, 2022

7c00 commented Jan 27, 2022

codope 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

7c00 commented Feb 14, 2022

pratyakshsharma commented Feb 20, 2022

codope commented Feb 21, 2022

7c00 commented Feb 25, 2022

codope commented Feb 25, 2022

arunthirupathi commented Feb 25, 2022

7c00 commented Feb 28, 2022

7c00 commented Mar 10, 2022

pratyakshsharma commented Mar 10, 2022

arunthirupathi commented Mar 10, 2022

7c00 commented Apr 11, 2022

codope commented May 2, 2022

7c00 commented May 25, 2022

pratyakshsharma commented Jun 7, 2022

arunthirupathi 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

codope commented Jun 13, 2022

arunthirupathi commented Jun 13, 2022

7c00 commented Jun 13, 2022

7c00 commented Jun 13, 2022

7c00 commented Jun 14, 2022

7c00 commented Dec 31, 2021 •

edited

Loading