Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add hudi integeration tests and bump version #17463

Merged
merged 2 commits into from
May 25, 2022
Merged

Add hudi integeration tests and bump version #17463

merged 2 commits into from
May 25, 2022

Conversation

7c00
Copy link
Member

@7c00 7c00 commented Mar 11, 2022 •
edited
Loading

This PR adds integration tests for querying Hudi tables. It could help prevent minor bugs when upgrading Hudi dependency versions.

The testing data is generated followed by the tutor at https://hudi.apache.org/docs/docker_demo (step1-step7).

This PR is extracted from #17149, and is to be shared by Hudi connector until Hudi connector gets production ready and the code for Hudi support is removed from Hive connector.

Test plan - Tests added.

== RELEASE NOTES ==
Hive Changes
* Upgrade Hudi support to 0.11.0

@7c00 7c00 force-pushed the hive-hudi-tests branch from 168d0bc to 1e1627e Compare March 11, 2022 10:05
@7c00 7c00 changed the title Add hudi integration tests Fix minor bugs and add integration test on querying hudi Mar 11, 2022
@7c00 7c00 mentioned this pull request Mar 16, 2022
Merged
@7c00 7c00 force-pushed the hive-hudi-tests branch 2 times, most recently from 569fb47 to c79bb33 Compare March 16, 2022 08:22
@7c00 7c00 changed the title Fix minor bugs and add integration test on querying hudi Add integration tests for querying Hudi tables Mar 16, 2022
@7c00 7c00 marked this pull request as ready for review March 16, 2022 08:32
@7c00
Copy link
Member Author

7c00 commented Mar 16, 2022

Some tests are expected to fail till #17477 merged.

@7c00 7c00 force-pushed the hive-hudi-tests branch from c79bb33 to 27a0661 Compare March 17, 2022 06:51
@7c00
Copy link
Member Author

7c00 commented Mar 17, 2022

@codope @pratyakshsharma @arunthirupathi would you like to take a review on this PR?

@7c00 7c00 mentioned this pull request Mar 17, 2022
Merged
@7c00
Copy link
Member Author

7c00 commented Mar 17, 2022

https://github.com/prestodb/presto/runs/5581838178?check_suite_focus=true

related issues:

@7c00
Copy link
Member Author

7c00 commented Mar 22, 2022

Holding. To be fixed after hudi 0.11 is released.

@7c00 7c00 mentioned this pull request Apr 11, 2022
Merged
@7c00 7c00 requested a review from a team as a code owner May 5, 2022 10:08
@7c00 7c00 requested a review from presto-oss May 5, 2022 10:08
@7c00 7c00 changed the title Add integration tests for querying Hudi tables Add hudi integeration tests and bump version May 5, 2022
@7c00 7c00 force-pushed the hive-hudi-tests branch 2 times, most recently from 379d7f0 to e931539 Compare May 6, 2022 08:54
@codope codope mentioned this pull request May 6, 2022
Closed
@7c00
Copy link
Member Author

7c00 commented May 6, 2022

@codope @pratyakshsharma @arunthirupathi

This PR is ready for review. Could you take a review on this PR?

codope
codope reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@codope codope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@7c00 Thanks for upgrading Hudi version and adding more coverage for Hudi tables. Overall looks good to me, just a few minor comments

presto-hive/src/main/java/com/facebook/presto/hive/util/HudiRealtimeSplitConverter.java Show resolved Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/hudi/HudiTestingDataGenerator.java Outdated Show resolved Hide resolved
@codope
Copy link
Contributor

codope commented May 11, 2022

@arunthirupathi Could you please take a look when you get a chance? This PR simply upgrades Hudi version, and adds more coverage for hudi table querying.

@7c00 7c00 force-pushed the hive-hudi-tests branch from e931539 to c6e7230 Compare May 11, 2022 15:21
@7c00
Copy link
Member Author

7c00 commented May 11, 2022

@codope Thanks for your comments. Have updated the code. Please take a second look.

codope
codope approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@codope codope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @7c00 for addressing the comments. Looks good to me.

@codope
Copy link
Contributor

codope commented May 17, 2022

@kewang1024 @arunthirupathi Can you please review this PR?

kewang1024
kewang1024 reviewed May 17, 2022
View reviewed changes
Copy link
Collaborator

@kewang1024 kewang1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #17149 gets merge, this PR's content needs to be deleted and moved to Hudi connector's specific directory right

Do we know the timeline for merging Hudi's connector?

presto-hive/src/test/java/com/facebook/presto/hive/hudi/HudiTestUtils.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/hudi/TestHudiIntegration.java
assertQuery(format(sqlTemplate, "stock_ticks_mor_rt"), sqlResult);
assertQuery(format(sqlTemplate, "stock_ticks_morn_ro"), sqlResultReadOptimized);
assertQuery(format(sqlTemplate, "stock_ticks_morn_rt"), sqlResult);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we comment somewhere the structures of those tables?
For other people, it seems hard to understand what we're testing against and can't verify if the expected the results are correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a doc hudi-testing-data.md to describe the testing data, and inserted a comment at the begining of HudiTestingDataGenerator#generateData to guide people to the doc.

@codope
Copy link
Contributor

codope commented May 19, 2022

If #17149 gets merge, this PR's content needs to be deleted and moved to Hudi connector's specific directory right

Do we know the timeline for merging Hudi's connector?

@kewang1024 Thanks for reviewing. The tests added here will continue to cover the current integration of Hudi with presto-hive connector. We will refactor some parts of this PR to enhance reusability across presto-hive and presto-hudi. Once we merge this PR, then we can rebase the presto-hudi connector PR and try to merge that asap contingent on reviews.

@kewang1024
Copy link
Collaborator

@7c00 let me know if the comments make sense?

@7c00 7c00 force-pushed the hive-hudi-tests branch from c6e7230 to a54a83d Compare May 23, 2022 08:54
@7c00
Copy link
Member Author

7c00 commented May 23, 2022

Hi, @kewang1024 Thanks for your comments! I have addressed the issues. Could you pelase take a second review?

@7c00 7c00 requested a review from kewang1024 May 23, 2022 09:00
kewang1024
kewang1024 approved these changes May 23, 2022
View reviewed changes
Copy link
Collaborator

@kewang1024 kewang1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the changes
One Nit: can you move the recent change to the first commit instead of the second commit
(removing the main function and adding test description file)

@kewang1024 kewang1024 requested review from highker and removed request for highker May 23, 2022 19:36
@7c00 7c00 force-pushed the hive-hudi-tests branch from a54a83d to edf68db Compare May 24, 2022 02:30
@7c00
Copy link
Member Author

7c00 commented May 24, 2022

Thanks @kewang1024. The two commits are refined.

Looks good to me, thanks for the changes One Nit: can you move the recent change to the first commit instead of the second commit (removing the main function and adding test description file)

@kewang1024 kewang1024 requested a review from highker May 24, 2022 03:53
@highker highker self-assigned this May 24, 2022
@7c00 7c00 force-pushed the hive-hudi-tests branch from edf68db to 9b90f13 Compare May 24, 2022 06:41
@7c00
Copy link
Member Author

7c00 commented May 24, 2022

@highker Thanks for your comments. I solved the issues mentioned. Lets merge this PR?

@7c00 7c00 force-pushed the hive-hudi-tests branch from 9b90f13 to e549232 Compare May 24, 2022 12:52
@highker highker merged commit 12dfba2 into prestodb:master May 25, 2022
@7c00
Copy link
Member Author

7c00 commented May 25, 2022

The PR is merged. Thanks @codope @kewang1024 @highker ❤️

@7c00 7c00 deleted the hive-hudi-tests branch May 25, 2022 01:47
@highker highker mentioned this pull request Jul 6, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kewang1024 kewang1024 kewang1024 approved these changes

@codope codope codope approved these changes

@highker highker highker approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@arunthirupathi arunthirupathi Awaiting requested review from arunthirupathi

Assignees

@highker highker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@7c00 @codope @kewang1024 @highker