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.

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

Add presto-native-tests module #23671

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

pramodsatya
Copy link
Contributor

@pramodsatya pramodsatya commented Sep 18, 2024

Description

Adds module presto-native-tests to run end-to-end tests with Presto native workers.

Motivation and Context

Improves Presto native test coverage by extending production tests from presto-tests module to use the native worker.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
== RELEASE NOTES ==
General Changes
* Add presto-native-tests module to run end-to-end tests with Presto native workers. :pr:`23671`

steveburnett
steveburnett previously approved these changes Sep 18, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Reviewed README.md, no concerns. Thanks for the doc!

@pramodsatya pramodsatya force-pushed the presto-native-tests branch 2 times, most recently from acaa071 to 8ec8208 Compare September 19, 2024 04:56
@pramodsatya pramodsatya marked this pull request as ready for review September 19, 2024 04:57
mknegi
mknegi previously approved these changes Sep 19, 2024
@tdcmeehan
Copy link
Contributor

Are these tests being run as part of the CI?

@pramodsatya
Copy link
Contributor Author

They are not being run as part of CI currently, but we plan to setup a pipeline to run these tests on a regular cadence subsequently. Will also add a CI job to run these tests in a follow-up PR.

public class TestAggregations
extends AbstractTestAggregationsNative
{
private static final String storageFormat = "PARQUET";
Copy link
Member

Choose a reason for hiding this comment

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

This is just testing parquet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the intention was to primarily test with parquet files. Most of the current e2e tests in presto-native-execution test with DWRF format. We can extend this to test with other file formats going forward.

@pramodsatya pramodsatya force-pushed the presto-native-tests branch 2 times, most recently from 32b2ad6 to f614cb6 Compare October 7, 2024 21:16
@pramodsatya pramodsatya force-pushed the presto-native-tests branch 3 times, most recently from 11d653c to 47ecc65 Compare November 11, 2024 07:49
@tdcmeehan tdcmeehan self-assigned this Nov 11, 2024
@steveburnett
Copy link
Contributor

Just a suggestions for the release note: Consider using the description of the PR in the release note entry to help a release note reader understand what this module does.

== RELEASE NOTES ==
General Changes
* Add presto-native-tests module to run end-to-end tests with Presto native workers. :pr:`23671`

@pramodsatya
Copy link
Contributor Author

Just a suggestions for the release note: Consider using the description of the PR in the release note entry to help a release note reader understand what this module does.

== RELEASE NOTES ==
General Changes
* Add presto-native-tests module to run end-to-end tests with Presto native workers. :pr:`23671`

Thanks for the suggestion @steveburnett, updated accordingly.

Copy link

linux-foundation-easycla bot commented Nov 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pramodsatya / name: Pramod Satya (c971a47)

@pramodsatya pramodsatya force-pushed the presto-native-tests branch 2 times, most recently from a3552e4 to c3e4729 Compare November 23, 2024 05:30
@pramodsatya pramodsatya marked this pull request as draft December 11, 2024 06:10
@pramodsatya pramodsatya force-pushed the presto-native-tests branch 4 times, most recently from 364a2bd to 31840cd Compare December 15, 2024 06:25
Co-authored-by: Manoj Negi <manojneg@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants