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

GH-55: [Gandiva] Re-enable tests #595

Merged
merged 8 commits into from
Feb 19, 2025
Merged

GH-55: [Gandiva] Re-enable tests #595

merged 8 commits into from
Feb 19, 2025

Conversation

lriggs
Copy link
Contributor

@lriggs lriggs commented Feb 8, 2025

Fixes GH-55.

Rationale for this change

I fixed the linking error here: apache/arrow#45114

What changes are included in this PR?

Reenabling some disabled tests.

Are these changes tested?

Yes, they are tests.

Are there any user-facing changes?

No.

@lriggs
Copy link
Contributor Author

lriggs commented Feb 8, 2025

How do you specify the arrow version used to build/test with arrow-java (so that I know it has my fix)? Does it always use the latest?

@kou kou changed the title GH-55 Renable tests after fixing link error. GH-55: Renable tests after fixing link error. Feb 8, 2025
@kou kou changed the title GH-55: Renable tests after fixing link error. GH-55: Renable tests after fixing link error Feb 8, 2025
@kou
Copy link
Member

kou commented Feb 8, 2025

We use the latest release for push/pull request:

- name: Download the latest Apache Arrow C++
if: github.event_name != 'schedule'
run: |
ci/scripts/download_cpp.sh

We use the main branch for schedule:

- name: Checkout Apache Arrow C++
if: github.event_name == 'schedule'
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
repository: apache/arrow
path: arrow

We need to update

VCPKG="943c5ef1c8f6b5e6ced092b242c8299caae2ff01" # 2024.04.26 Release
to use vcpkg configuration introduced by apache/arrow#45114 .

@lriggs
Copy link
Contributor Author

lriggs commented Feb 12, 2025

Thanks. I updated vcpkg as well now.

@lidavidm
Copy link
Member

Looks like one more minor CI nit:

  Error:  /home/runner/work/arrow-java/arrow-java/gandiva/src/test/java/org/apache/arrow/gandiva/evaluator/FilterProjectTest.java:37:8: Unused import: org.junit.jupiter.api.Disabled. [UnusedImports]

@kou kou changed the title GH-55: Renable tests after fixing link error GH-55: [Gandiva] Renable tests after fixing link error Feb 12, 2025
@kou kou changed the title GH-55: [Gandiva] Renable tests after fixing link error GH-55: [Gandiva] Renable tests Feb 12, 2025
@lidavidm
Copy link
Member

There's others - are you able to run the build locally at all to test?

   Error:  /home/runner/work/arrow-java/arrow-java/gandiva/src/test/java/org/apache/arrow/gandiva/evaluator/FilterTest.java:37:8: Unused import: org.junit.jupiter.api.Disabled. [UnusedImports]
  Error:  /home/runner/work/arrow-java/arrow-java/gandiva/src/test/java/org/apache/arrow/gandiva/evaluator/ProjectorDecimalTest.java:45:8: Unused import: org.junit.jupiter.api.Disabled. [UnusedImports]

@lriggs lriggs requested a review from kou as a code owner February 13, 2025 00:44
@kou kou changed the title GH-55: [Gandiva] Renable tests GH-55: [Gandiva] Re-enable tests Feb 13, 2025
@lriggs
Copy link
Contributor Author

lriggs commented Feb 13, 2025

Now its failing with "Gandiva Failed to make LLVM module due to Could not create LLJIT instance: Symbols not found: [ llvm_orc_registerEHFrameSectionWrapper ]" which is what should be fixed in my other change in apache/arrow.

Seems to be using the 18.2 jars: /home/runner/.m2/repository/org/apache/arrow/arrow-avro/18.2.0/

@lidavidm
Copy link
Member

Ah, the CI here uses the latest released version of the C++ libraries, not the development version...

@lidavidm
Copy link
Member

We could either make the skips conditional on the version of the C++ code somehow, or otherwise just always test against the dev release?

@kou
Copy link
Member

kou commented Feb 13, 2025

C++ version must not be related.
apache/arrow#45114 just updated LLVM version.

@lidavidm
Copy link
Member

@kou
Copy link
Member

kou commented Feb 13, 2025

Ah, I thought that updating VCPKG in .env is enough to use LLVM 18 but we refer ci/vcpkg in apache/arrow here...:

--x-manifest-root=/arrow/ci/vcpkg \

The log shows that we use LLVM 17:

https://github.com/apache/arrow-java/actions/runs/13298859368/job/37137015507?pr=595#step:10:1663

  -- Found LLVMAlt: 17.0.2

OK. Let's use main for now:

diff --git a/.github/workflows/rc.yml b/.github/workflows/rc.yml
index faad5983..f2666a31 100644
--- a/.github/workflows/rc.yml
+++ b/.github/workflows/rc.yml
@@ -299,13 +299,13 @@ jobs:
         shell: bash
         run: |
           tar -xf apache-arrow-java-*.tar.gz --strip-components=1
-      - name: Download the latest Apache Arrow C++
-        if: github.event_name != 'schedule'
-        shell: bash
-        run: |
-          ci/scripts/download_cpp.sh
+      # - name: Download the latest Apache Arrow C++
+      #   if: github.event_name != 'schedule'
+      #   shell: bash
+      #   run: |
+      #     ci/scripts/download_cpp.sh
       - name: Checkout Apache Arrow C++
-        if: github.event_name == 'schedule'
+        # if: github.event_name == 'schedule'
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
         with:
           repository: apache/arrow

@lidavidm
Copy link
Member

Ah, I see what you meant by the original comment now...

@lriggs
Copy link
Contributor Author

lriggs commented Feb 13, 2025

I'll try your suggested change @kou

@lriggs
Copy link
Contributor Author

lriggs commented Feb 13, 2025

Latest failures were because I only updated the windows build in rc.yml. Just fixed that for mac and linux.

@kou
Copy link
Member

kou commented Feb 14, 2025

Oh, sorry. It's my fault...
We just need to use main only for Linux because Linux build only uses vcpkg to install LLVM.

@lriggs
Copy link
Contributor Author

lriggs commented Feb 14, 2025

Strange, it still seems to be using the old vcpkg.

Does arrow-java need something like this? https://github.com/apache/arrow/blob/main/ci/scripts/install_vcpkg.sh

2025-02-14T00:07:03.2003955Z CMake Warning at /opt/vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
2025-02-14T00:07:03.2004967Z   Could not find a configuration file for package "LLVM" that is compatible
2025-02-14T00:07:03.2005618Z   with requested version "19.1".

....

2025-02-14T00:07:03.2600773Z -- Found LLVMAlt: 17.0.2
2025-02-14T00:07:03.2601530Z -- Using LLVMConfig.cmake in: /opt/vcpkg/installed/amd64-linux-static-release/share/llvm

@kou
Copy link
Member

kou commented Feb 16, 2025

Ah, we need to expire a cache when we change vcpkg version. Could you try this?

diff --git a/compose.yaml b/compose.yaml
index 58a19676..b125c3c9 100644
--- a/compose.yaml
+++ b/compose.yaml
@@ -92,12 +92,12 @@ services:
     # Usage:
     #   docker compose build vcpkg-jni
     #   docker compose run vcpkg-jni
-    image: ${REPO}:${ARCH}-vcpkg-jni
+    image: ${REPO}:${ARCH}-vcpkg-jni-${VCPKG}
     build:
       context: .
       dockerfile: ci/docker/vcpkg-jni.dockerfile
       cache_from:
-        - ${REPO}:${ARCH}-vcpkg-jni
+        - ${REPO}:${ARCH}-vcpkg-jni-${VCPKG}
       args:
         base: ${ARROW_REPO}:${ARCH}-python-${PYTHON}-wheel-manylinux-2014-vcpkg-${VCPKG}
     volumes:

@lidavidm lidavidm added the bug-fix PRs that fix a big. label Feb 18, 2025
@lriggs
Copy link
Contributor Author

lriggs commented Feb 18, 2025

Ok, just pushed a change to expire the cache. Hopefully it works.

@lidavidm
Copy link
Member

Looks like that worked!

@lidavidm lidavidm added this to the 18.3.0 milestone Feb 19, 2025
@lidavidm lidavidm merged commit e1dd6d8 into apache:main Feb 19, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix PRs that fix a big.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Java] Renable Disabled Gandiva Tests after fixing the linking error
3 participants