Add presto-native-tests module

Co-authored-by: Manoj Negi <[email protected]>
prestodb · Dec 16, 2024 · 171afbd · 171afbd
1 parent 17ac4c3
commit 171afbd
Show file tree

Hide file tree

Showing 25 changed files with 3,194 additions and 60 deletions.
diff --git a/.circleci/continue_config.yml b/.circleci/continue_config.yml
@@ -29,6 +29,10 @@ workflows:
           run_linux_tests: << pipeline.parameters.run_linux_tests >>
           requires:
             - linux-build-and-unit-test
+      - linux-presto-native-tests:
+          run_linux_tests: << pipeline.parameters.run_linux_tests >>
+          requires:
+            - linux-build-and-unit-test
       - linux-spark-e2e-tests:
           run_linux_tests: << pipeline.parameters.run_linux_tests >>
           requires:
@@ -198,6 +202,54 @@ jobs:
             - store_artifacts:
                 path: '/tmp/PrestoNativeQueryRunnerUtils'
 
+  linux-presto-native-tests:
+    executor: build
+    parameters:
+      run_linux_tests:
+        type: boolean
+        default: false
+    parallelism: 5
+    steps:
+      - run: echo "Run Linux tests is << parameters.run_linux_tests >>"
+      - when:
+          condition: << parameters.run_linux_tests >>
+          steps:
+            - checkout
+            - attach_workspace:
+                at: presto-native-execution
+            - maven_install:
+                maven_install_opts: ${MAVEN_INSTALL_OPTS}
+                maven_fast_install: ${MAVEN_FAST_INSTALL}
+            - run:
+                name: 'Run Presto native tests'
+                command: |
+                  export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/usr/local/lib:/usr/local/lib64"
+                  export PRESTO_SERVER_PATH="${HOME}/project/presto-native-execution/_build/debug/presto_cpp/main/presto_server"
+                  export TEMP_PATH="/tmp"
+                  TESTFILES=$(circleci tests glob "presto-native-tests/src/test/**/Test*.java" | circleci tests split --split-by=timings)
+                  # Convert file paths to comma separated class names
+                  export TESTCLASSES=
+                  for test_file in $TESTFILES
+                  do
+                    tmp=${test_file##*/}
+                    test_class=${tmp%%\.*}
+                    export TESTCLASSES="${TESTCLASSES},$test_class"
+                  done
+                  export TESTCLASSES=${TESTCLASSES#,}
+                  
+                  if [ ! -z $TESTCLASSES ]; then
+                    mvn test \
+                      ${MAVEN_TEST} \
+                      -pl 'presto-native-tests' \
+                      -Dtest="${TESTCLASSES}" \
+                      -DPRESTO_SERVER=${PRESTO_SERVER_PATH} \
+                      -DDATA_DIR=${TEMP_PATH} \
+                      -Duser.timezone=America/Bahia_Banderas \
+                      -T1C
+                  fi
+            - store_artifacts:
+                path: '/tmp/PrestoNativeQueryRunnerUtils'
+
   linux-presto-native-sidecar-tests:
     executor: build
     parameters:

diff --git a/.github/workflows/prestocpp-linux-build-and-unit-test.yml b/.github/workflows/prestocpp-linux-build-and-unit-test.yml
@@ -169,6 +169,88 @@ jobs:
             -Duser.timezone=America/Bahia_Banderas \
             -T1C
 
+
+  prestocpp-linux-presto-native-tests:
+    needs: prestocpp-linux-build-for-test
+    runs-on: ubuntu-22.04
+    container:
+      image: prestodb/presto-native-dependency:0.290-20241014120930-e1fc090
+    env:
+      MAVEN_OPTS: "-Xmx4G -XX:+ExitOnOutOfMemoryError"
+      MAVEN_FAST_INSTALL: "-B -V --quiet -T 1C -DskipTests -Dair.check.skip-all -Dmaven.javadoc.skip=true"
+      MAVEN_TEST: "-B -Dair.check.skip-all -Dmaven.javadoc.skip=true -DLogTestDurationListener.enabled=true --fail-at-end"
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Fix git permissions
+        # Usually actions/checkout does this but as we run in a container
+        # it doesn't work
+        run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
+
+      - name: Download artifacts
+        uses: actions/download-artifact@v4
+        with:
+          name: presto-native-build
+          path: presto-native-execution/_build/release
+
+      # Permissions are lost when uploading. Details here: https://github.com/actions/upload-artifact/issues/38
+      - name: Restore execute permissions and library path
+        run: |
+          chmod +x ${GITHUB_WORKSPACE}/presto-native-execution/_build/release/presto_cpp/main/presto_server
+          chmod +x ${GITHUB_WORKSPACE}/presto-native-execution/_build/release/velox/velox/functions/remote/server/velox_functions_remote_server_main
+          # Ensure transitive dependency libboost-iostreams is found.
+          ldconfig /usr/local/lib
+
+      - name: Install OpenJDK8
+        uses: actions/setup-java@v4
+        with:
+          distribution: 'temurin'
+          java-version: '8'
+
+      - name: Cache local Maven repository
+        id: cache-maven
+        uses: actions/cache@v4
+        with:
+          path: ~/.m2/repository
+          key: ${{ runner.os }}-maven-2-${{ hashFiles('**/pom.xml') }}
+          restore-keys: |
+            ${{ runner.os }}-maven-2-
+
+      - name: Populate maven cache
+        if: steps.cache-maven.outputs.cache-hit != 'true'
+        run: ./mvnw de.qaware.maven:go-offline-maven-plugin:resolve-dependencies
+
+      - name: Maven install
+        env:
+          # Use different Maven options to install.
+          MAVEN_OPTS: "-Xmx2G -XX:+ExitOnOutOfMemoryError"
+        run: |
+          for i in $(seq 1 3); do ./mvnw clean install $MAVEN_FAST_INSTALL -pl 'presto-native-tests' -am && s=0 && break || s=$? && sleep 10; done; (exit $s)
+
+      - name: Run presto-native tests
+        run: |
+          export PRESTO_SERVER_PATH="${GITHUB_WORKSPACE}/presto-native-execution/_build/release/presto_cpp/main/presto_server"
+          export TESTFILES=`find ./presto-native-tests/src/test -type f -name 'Test*.java'`
+          # Convert file paths to comma separated class names
+          export TESTCLASSES=
+          for test_file in $TESTFILES
+          do
+            tmp=${test_file##*/}
+            test_class=${tmp%%\.*}
+            export TESTCLASSES="${TESTCLASSES},$test_class"
+          done
+          export TESTCLASSES=${TESTCLASSES#,}
+          echo "TESTCLASSES = $TESTCLASSES"
+
+          mvn test \
+            ${MAVEN_TEST} \
+            -pl 'presto-native-tests' \
+            -Dtest="${TESTCLASSES}" \
+            -DPRESTO_SERVER=${PRESTO_SERVER_PATH} \
+            -DDATA_DIR=${RUNNER_TEMP} \
+            -Duser.timezone=America/Bahia_Banderas \
+            -T1C
+
   prestocpp-linux-spark-e2e-tests:
     needs: prestocpp-linux-build-for-test
     runs-on: ubuntu-22.04

diff --git a/.github/workflows/test-other-modules.yml b/.github/workflows/test-other-modules.yml
@@ -64,6 +64,7 @@ jobs:
         run: |
           ./mvnw test -T 1 ${MAVEN_TEST} -pl '
             !presto-tests, 
+            !presto-native-tests,
             !presto-accumulo,
             !presto-cassandra,
             !presto-hive,

diff --git a/pom.xml b/pom.xml
@@ -197,6 +197,7 @@
         <module>presto-test-coverage</module>
         <module>presto-hudi</module>
         <module>presto-native-execution</module>
+        <module>presto-native-tests</module>
         <module>presto-router</module>
         <module>presto-open-telemetry</module>
         <module>redis-hbo-provider</module>

diff --git a/...xecution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java b/...xecution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
@@ -43,7 +43,9 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.UUID;
@@ -121,7 +123,7 @@ public static QueryRunner createQueryRunner(
 
         defaultQueryRunner.close();
 
-        return createNativeQueryRunner(dataDirectory.get().toString(), prestoServerPath.get(), workerCount, cacheMaxSize, true, Optional.empty(), storageFormat, addStorageFormatToPath, false, isCoordinatorSidecarEnabled, false);
+        return createNativeQueryRunner(dataDirectory.get().toString(), prestoServerPath.get(), workerCount, cacheMaxSize, true, Optional.empty(), storageFormat, addStorageFormatToPath, false, isCoordinatorSidecarEnabled, false, Collections.emptyMap(), Collections.emptyMap());
     }
 
     public static QueryRunner createJavaQueryRunner()
@@ -326,7 +328,9 @@ public static QueryRunner createNativeQueryRunner(
             boolean addStorageFormatToPath,
             Boolean failOnNestedLoopJoin,
             boolean isCoordinatorSidecarEnabled,
-            boolean singleNodeExecutionEnabled)
+            boolean singleNodeExecutionEnabled,
+            Map<String, String> extraProperties,
+            Map<String, String> extraCoordinatorProperties)
             throws Exception
     {
         // The property "hive.allow-drop-table" needs to be set to true because security is always "legacy" in NativeQueryRunner.
@@ -350,8 +354,9 @@ public static QueryRunner createNativeQueryRunner(
                         .put("experimental.internal-communication.thrift-transport-enabled", String.valueOf(useThrift))
                         .putAll(getNativeWorkerSystemProperties())
                         .putAll(isCoordinatorSidecarEnabled ? getNativeSidecarProperties() : ImmutableMap.of())
+                        .putAll(extraProperties)
                         .build(),
-                coordinatorProperties.build(),
+                coordinatorProperties.putAll(extraCoordinatorProperties).build(),
                 "legacy",
                 hiveProperties,
                 workerCount,
@@ -408,6 +413,28 @@ public static QueryRunner createNativeQueryRunner(String remoteFunctionServerUds
         return createNativeQueryRunner(false, DEFAULT_STORAGE_FORMAT, Optional.ofNullable(remoteFunctionServerUds), false, false, false);
     }
 
+    public static QueryRunner createNativeQueryRunner(Map<String, String> extraProperties, Map<String, String> extraCoordinatorProperties,
+                                                      String storageFormat)
+            throws Exception
+    {
+        int cacheMaxSize = 0;
+        NativeQueryRunnerParameters nativeQueryRunnerParameters = getNativeQueryRunnerParameters();
+        return createNativeQueryRunner(
+                nativeQueryRunnerParameters.dataDirectory.toString(),
+                nativeQueryRunnerParameters.serverBinary.toString(),
+                nativeQueryRunnerParameters.workerCount,
+                cacheMaxSize,
+                true,
+                Optional.empty(),
+                storageFormat,
+                true,
+                false,
+                false,
+                false,
+                extraProperties,
+                extraCoordinatorProperties);
+    }
+
     public static QueryRunner createNativeQueryRunner(boolean useThrift)
             throws Exception
     {
@@ -448,7 +475,9 @@ public static QueryRunner createNativeQueryRunner(
                 true,
                 failOnNestedLoopJoin,
                 isCoordinatorSidecarEnabled,
-                singleNodeExecutionEnabled);
+                singleNodeExecutionEnabled,
+                Collections.emptyMap(),
+                Collections.emptyMap());
     }
 
     // Start the remote function server. Return the UDS path used to communicate with it.

diff --git a/presto-native-tests/README.md b/presto-native-tests/README.md
@@ -0,0 +1,31 @@
+# Presto Native Tests
+
+This module contains end-to-end tests that run queries from test classes in 
+the `presto-tests` module with Presto C++ workers. Please build the module
+`presto-native-execution` first. 
+
+The following command can be used to run all tests in this module:
+```
+mvn test 
+    -pl 'presto-native-tests' 
+    -Dtest="com.facebook.presto.nativetests.Test*" 
+    -Duser.timezone=America/Bahia_Banderas 
+    -DPRESTO_SERVER=${PRESTO_HOME}/presto-native-execution/cmake-build-debug/presto_cpp/main/presto_server 
+    -DWORKER_COUNT=${WORKER_COUNT} -T1C
+```
+Please update JVM argument `PRESTO_SERVER` to point to the Presto C++ worker
+binary `presto_server`. 
+
+## Adding new tests
+
+Presto C++ currently does not have the same behavior as Presto for certain 
+queries. This could be because of missing types, missing function signatures,
+among other reasons. Tests with these unsupported queries are therefore 
+expected to fail and the test asserts the error message is as expected. 
+
+Issues should also be created for the failing queries, so they are documented
+and fixed. Please add the tag `presto-native-tests` for these issues. 
+Once all the failures in a testcase are fixed, the overriden test in this 
+module should be removed and the testcase in the corresponding base class in
+`presto-tests` would be the single source of truth for Presto SQL coverage 
+tests.