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

[native] Introduce module presto-native-tests #24234

Open

pramodsatya wants to merge 2 commits into prestodb:master from pramodsatya:add_native_tests

Contributor

pramodsatya commented Dec 10, 2024 •

edited

Loading

Description

Introduces module presto-native-tests to run end-to-end tests with Presto native workers. Adds Github action to run these tests.

Motivation and Context

Improves Presto native test coverage by extending production tests from presto-tests module to use the native worker. For full context please refer to #23671.

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
* Introduces module presto-native-tests to run end-to-end tests with Presto native workers. :pr:`24234`

pramodsatya requested a review from aditi-pandit

December 10, 2024 15:25

pramodsatya requested review from steveburnett, a team, jaystarshot, feilong-liu, elharo and ClarenceThreepwood as code owners

December 10, 2024 15:25

pramodsatya requested a review from presto-oss

December 10, 2024 15:25

prestodb-ci added the from:IBM label

prestodb-ci requested review from a team and ScrapCodes and removed request for a team

December 10, 2024 15:25

pramodsatya force-pushed the add_native_tests branch 2 times, most recently from 6f3ef6f to ceeabce Compare

December 15, 2024 06:31

steveburnett requested changes

View reviewed changes

Contributor

steveburnett left a comment

Thank you for the documentation! I made some suggestions for ease of reading and clarity.

If you feel that any of my suggestions change the meaning in a way that is not correct, let me know and we can work on them together.

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

pramodsatya force-pushed the add_native_tests branch from ceeabce to 8f606ce Compare

January 14, 2025 19:05

pramodsatya commented

View reviewed changes

Contributor Author

pramodsatya left a comment

Thanks for the feedback @steveburnett, addressed the comments, could you please take another look?

presto-native-tests/README.md Show resolved Hide resolved

presto-native-tests/README.md Show resolved Hide resolved

steveburnett previously approved these changes

View reviewed changes

Contributor

steveburnett left a comment

LGTM!

Thanks for addressing the comments! Looks good.

aditi-pandit reviewed

View reviewed changes

Contributor

aditi-pandit left a comment

@pramodsatya : Lets move the second commit to refactor presto-tests in a separate PR. That would make it easier to review. Lets add this work as a motivation for that refactoring.

presto-native-tests/pom.xml

+                      <dependency>
+                          <groupId>com.facebook.presto</groupId>
+                          <artifactId>presto-tests</artifactId>
+                          <version>${project.version}</version>

Contributor

aditi-pandit Jan 16, 2025

Why is this particular module tried to ${project.version} and not other modules ?

Contributor Author

pramodsatya Jan 17, 2025

Thanks for pointing it out, added for other modules as well.

...-tests/src/test/java/com.facebook.presto.nativetests/TestNonIterativeDistributedQueries.java Outdated

+                  protected QueryRunner createQueryRunner() throws Exception
+                  {
+                      return PrestoNativeQueryRunnerUtils.createNativeQueryRunner(
+                              ImmutableMap.of("experimental.iterative-optimizer-enabled", "false"),

Contributor

aditi-pandit Jan 16, 2025

What does this do ? Its important to specify why this test configuration matters.

...-tests/src/test/java/com.facebook.presto.nativetests/TestNonIterativeDistributedQueries.java Outdated

+                  }
+                  @Override
+                  protected void createTables()

Contributor

aditi-pandit Jan 16, 2025

Can this function be moved to AbstractTestQueriesNative ?

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                      assertQuery("select reduce_agg(x, array[], (x, y)->array[element_at(x, 2)],  (x, y)->array[element_at(x, 2)]) from (select array[array[1]]) T(x)", "select array[null]");
+                  }
+                  // The following approx_set function tests are overriden since approx_set returns different results in Presto C++.

Contributor

aditi-pandit Jan 16, 2025

Do we have any outward article or documentation for this ? If not, then we should write one.

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  @Test
+                  public void testArrayCumSumDecimals()
+                  {
+                      String functionNotRegisteredError = ".*Scalar function presto.default.array_cum_sum not registered with arguments.*";

Contributor

aditi-pandit Jan 16, 2025

Do we have this error because we don't have this functionality ? I don't see a reason for not building it. Do we have an issue for this ? Please create one if its not there and add the link here.

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  @Override
+                  @Test
+                  public void testDuplicateUnnestRows()

Contributor

aditi-pandit Jan 16, 2025

Do we have an issue for this ?

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  @Override
+                  @Test
+                  public void testSamplingJoinChain()

Contributor

aditi-pandit Jan 16, 2025

key_sampling seems like an important feature. Do we have an issue ?

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  @Override
+                  @Test
+                  public void testScalarSubquery()

Contributor

aditi-pandit Jan 16, 2025

Why is this test separated from the java one ?

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  @Override
+                  @Test
+                  public void testLikePrefixAndSuffixWithChars()

Contributor

aditi-pandit Jan 16, 2025

These failures are because CHAR Is not supported. Lets add the issue here.

But also any reason these are written with CHAR and not VARCHAR ? We could try to split the original test.

...to-native-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestQueriesNative.java Outdated

+                  }
+                  @Override
+                  public void testMergeKHyperLogLog()

Contributor

aditi-pandit Jan 16, 2025

Add an issue for the type not supported and link it here.

prestodb deleted a comment from aditi-pandit

prestodb deleted a comment from aditi-pandit


          Refactor testcases for presto-native-tests

0d089d8

pramodsatya dismissed steveburnett’s stale review via

a42b0c7

January 22, 2025 07:06

pramodsatya force-pushed the add_native_tests branch from 8f606ce to a42b0c7 Compare

January 22, 2025 07:06

pramodsatya commented

View reviewed changes

Contributor Author

pramodsatya left a comment

Thanks @aditi-pandit, addressed the comments and created missing issues. Also modified this PR to only include test classes with fewer modifications as discussed. Will open follow-up PRs for the remaining larger test classes from #23671, such as AbstractTestQueriesNative.java.
Could you please take another look?

presto-native-tests/pom.xml

+                      <dependency>
+                          <groupId>com.facebook.presto</groupId>
+                          <artifactId>presto-tests</artifactId>
+                          <version>${project.version}</version>

Contributor Author

pramodsatya Jan 17, 2025

Thanks for pointing it out, added for other modules as well.

pramodsatya force-pushed the add_native_tests branch from a42b0c7 to bdd1b9f Compare

January 22, 2025 09:39


          [native] Introduce module presto-native-tests

7e62480

Co-authored-by: Manoj Negi <[email protected]>

pramodsatya force-pushed the add_native_tests branch from bdd1b9f to 7e62480 Compare

January 23, 2025 03:59

aditi-pandit reviewed

View reviewed changes

Contributor

aditi-pandit left a comment

Thanks @pramodsatya

presto-native-tests/src/test/java/com.facebook.presto.nativetests/TestOrderByQueries.java

+                  private static final String storageFormat = "PARQUET";
+                  @Override
+                  protected QueryRunner createQueryRunner() throws Exception

Contributor

aditi-pandit Jan 23, 2025

Do we plan to overload the expected query runner with the JavaQueryRunner as well ?

...ests/src/test/java/com.facebook.presto.nativetests/TestRepartitionQueriesWithSmallPages.java

+                  @Override
+                  protected QueryRunner createQueryRunner() throws Exception
+                  {
+                      return PrestoNativeQueryRunnerUtils.createNativeQueryRunner(

Contributor

aditi-pandit Jan 23, 2025

Does native engine support these session properties ? Will native side-car catch these ?

.github/workflows/prestocpp-linux-build-and-unit-test.yml

+                  needs: prestocpp-linux-build-for-test
+                  runs-on: ubuntu-22.04
+                  container:
+                    image: prestodb/presto-native-dependency:0.290-20241014120930-e1fc090

Contributor

aditi-pandit Jan 23, 2025

Can we remove this hardcoded version ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aditi-pandit aditi-pandit left review comments

steveburnett steveburnett left review comments

jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

elharo Awaiting requested review from elharo elharo is a code owner

ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

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

ScrapCodes Awaiting requested review from ScrapCodes ScrapCodes was automatically assigned from prestodb/ibm-reviewers

At least 1 approving review is required to merge this pull request.

Labels