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 hive4 query runner and tests #24418

Merged
merged 8 commits into from
Dec 19, 2024
Merged

Add hive4 query runner and tests #24418

merged 8 commits into from
Dec 19, 2024
+2,938 −2,369

Conversation

mayankvadariya
Copy link
Contributor

@mayankvadariya mayankvadariya commented Dec 9, 2024 •
edited
Loading

Description

Add hive4 query runner and tests. Depends on trinodb/docker-images#218

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 9, 2024
@github-actions github-actions bot added iceberg Iceberg connector hive Hive connector labels Dec 9, 2024
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 2 times, most recently from 7891c62 to bc7f174 Compare December 10, 2024 17:10
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from bc7f174 to cba0c2d Compare December 10, 2024 17:21
anusudarsan
anusudarsan reviewed Dec 10, 2024
View reviewed changes
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4HiveServer.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4Metastore.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4MinioDataLake.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive4OnDataLake.java Outdated Show resolved Hide resolved
...rc/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHive4CatalogWithHiveMetastore.java Outdated Show resolved Hide resolved
...rc/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHive4CatalogWithHiveMetastore.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive4OnDataLake.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive4OnDataLake.java Outdated Show resolved Hide resolved
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 3 times, most recently from 4b7d6a2 to c235742 Compare December 11, 2024 04:18
hashhar
hashhar reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % Anu's comments.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java Show resolved Hide resolved
@ebyhr
Copy link
Member

ebyhr commented Dec 12, 2024

https://github.com/trinodb/docker-images release is complete.

@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 4 times, most recently from 4d83587 to b5b5031 Compare December 12, 2024 23:25
@github-actions github-actions bot added hudi Hudi connector delta-lake Delta Lake connector labels Dec 12, 2024
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 2 times, most recently from f40a86c to b019fee Compare December 13, 2024 02:57
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 2 times, most recently from c08f7b5 to c20f3e0 Compare December 13, 2024 19:51
anusudarsan
anusudarsan reviewed Dec 13, 2024
View reviewed changes
...a-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeFlushMetadataCacheProcedure.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/HiveMinioDataLake.java

public HiveMinioDataLake(String bucketName)
{
this.bucketName = requireNonNull(bucketName, "bucketName is null");
Copy link
Member

Choose a reason for hiding this comment

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

instead of passing bucketname, can you simply create unique buckets using + randomSuffix() here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel that bucket name is taken as an input to provide meaningful bucket name by caller as HiveMinioDataLake is used across classes.

Copy link
Member

Choose a reason for hiding this comment

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

by caller as HiveMinioDataLake is used across classes.

ya but does not look like Test... CatalogWithHiveMetastore does the same..nbd

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive3OnDataLake.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive4OnDataLake.java Outdated Show resolved Hide resolved
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch 2 times, most recently from c38b2b6 to 4305438 Compare December 15, 2024 03:22
@mayankvadariya
Copy link
Contributor Author

@hashhar please review.

@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from 4305438 to 22e3e83 Compare December 16, 2024 14:40
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from 22e3e83 to 3adb961 Compare December 16, 2024 16:48
anusudarsan
anusudarsan reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@anusudarsan anusudarsan left a comment

Choose a reason for hiding this comment

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

few nits. LGTM

plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4Metastore.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4HiveServer.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4HiveServer.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/Hive4Metastore.java Show resolved Hide resolved
plugin/trino-hive/src/test/resources/hive_minio_datalake/hive4-hive-site.xml Outdated Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/containers/HiveMinioDataLake.java

public HiveMinioDataLake(String bucketName)
{
this.bucketName = requireNonNull(bucketName, "bucketName is null");
Copy link
Member

Choose a reason for hiding this comment

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

by caller as HiveMinioDataLake is used across classes.

ya but does not look like Test... CatalogWithHiveMetastore does the same..nbd

@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from 3adb961 to b6993e7 Compare December 17, 2024 23:47
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from b6993e7 to 483d071 Compare December 18, 2024 22:41
@mayankvadariya mayankvadariya force-pushed the mayank/add-hive4-query-runner-and-tests branch from 483d071 to 2dfb20c Compare December 19, 2024 01:55
@mayankvadariya
Copy link
Contributor Author

https://github.com/trinodb/trino/actions/runs/12404758413/job/34630462973?pr=24418 looks like a unrelated failure.

@mayankvadariya mayankvadariya requested a review from ebyhr December 19, 2024 04:02
@ebyhr ebyhr merged commit 1bd8b3a into trinodb:master Dec 19, 2024
100 checks passed
@github-actions github-actions bot added this to the 469 milestone Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findinpath findinpath findinpath left review comments

@anusudarsan anusudarsan anusudarsan left review comments

@hashhar hashhar Awaiting requested review from hashhar

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector hive Hive connector hudi Hudi connector iceberg Iceberg connector
Milestone
469
Development

Successfully merging this pull request may close these issues.
5 participants
@mayankvadariya @ebyhr @hashhar @anusudarsan @findinpath