GH-38697: [C++][Gandiva] Use arrow io util to replace std::filesystem::path in gandiva by niyue · Pull Request #38698 · apache/arrow

niyue · 2023-11-14T01:39:43Z

Rationale for this change

AlmaLinux 8 CI reported linker failure when std::filesystem::path is used, and This PR tries to it.

What changes are included in this PR?

Replace std::filesystem::path in Gandiva with Arrow's internal io util so that AlmaLinux 8 CI build can work.

Are these changes tested?

It should be covered by existing tests and CI.

Are there any user-facing changes?

No

Closes: [C++][Gandiva] AlmaLinux 8 CI build failed due to std::filesystem::path usage #38697

github-actions · 2023-11-14T01:40:10Z

⚠️ GitHub issue #38697 has been automatically assigned in GitHub to PR creator.

niyue · 2023-11-14T01:41:54Z

cpp/src/gandiva/tests/test_util.cc

Previously this DefaultConfiguration function is called by creating a new instance of ConfigurationBuilder, but I think this is not necessary since it is a static function of ConfigurationBuilder, so I change it as well.

pitrou · 2023-11-14T16:31:23Z

This PR tries to fix #38697

It would be better to make the PR description self-contained, can you update this?

pitrou · 2023-11-14T16:33:00Z

@github-actions crossbow submit -g cpp

pitrou · 2023-11-14T17:00:32Z

@niyue There are Windows failures that need fixing.

niyue · 2023-11-15T02:24:21Z

It would be better to make the PR description self-contained

Sure. Updated.

js8544 · 2023-11-15T03:08:19Z

The AppVeyor check is still failing: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/48524728. std::wstring can't be constructed directly without encoding.
Perhaps we should use PlatformFilename::FromString and DCHECK the result instead?

…il so that AlmaLinux 8 won't complain.

niyue · 2023-11-15T06:39:02Z

Perhaps we should use PlatformFilename::FromString and DCHECK the result instead?

Thanks for the suggestion. I took the approach and it did work. And Windows build should be okay now.

js8544

+1. Thanks!

pitrou · 2023-11-15T09:48:40Z

@github-actions crossbow submit -g cpp

pitrou · 2023-11-15T09:48:50Z

@github-actions crossbow submit almalinux

github-actions · 2023-11-15T09:51:14Z

Revision: 5ae2764

Submitted crossbow builds: ursacomputing/crossbow @ actions-fe6233792a

Task	Status
almalinux-8-amd64
almalinux-8-arm64
almalinux-9-amd64
almalinux-9-arm64
verify-rc-binaries-jars-linux-almalinux-8-amd64
verify-rc-binaries-wheels-linux-almalinux-8-amd64
verify-rc-source-cpp-linux-almalinux-8-amd64
verify-rc-source-csharp-linux-almalinux-8-amd64
verify-rc-source-go-linux-almalinux-8-amd64
verify-rc-source-integration-linux-almalinux-8-amd64
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-js-linux-almalinux-8-amd64
verify-rc-source-python-linux-almalinux-8-amd64
verify-rc-source-ruby-linux-almalinux-8-amd64

github-actions · 2023-11-15T09:51:20Z

Revision: 5ae2764

Submitted crossbow builds: ursacomputing/crossbow @ actions-d28ae2b20a

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

cpp/src/gandiva/tests/test_util.cc

Co-authored-by: Antoine Pitrou <[email protected]>

pitrou · 2023-11-15T15:06:58Z

Sorry for the misleading code suggestion. I've pushed a fix.

conbench-apache-arrow · 2023-11-15T21:44:35Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 41e45fe.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

niyue · 2023-11-16T01:47:53Z

Sorry for the misleading code suggestion

Sorry I should really verify this on my mac. I happened to use GitHub iOS app on the iPad yesterday night when I saw this, and I found the first time that the app provides a feature allowing me to apply the fix and resolve the conversation directly without opening any text editor so I gave it a try. Thanks for the fix.

…system::path in gandiva (apache#38698) ### Rationale for this change AlmaLinux 8 CI reported linker failure when `std::filesystem::path` is used, and This PR tries to it. ### What changes are included in this PR? Replace replace `std::filesystem::path` in gandiva with arrow's internal io util so that AlmaLinux 8 CI build can work. ### Are these changes tested? It should be covered by existing tests and CI. ### Are there any user-facing changes? No * Closes: apache#38697 Lead-authored-by: Yue Ni <[email protected]> Co-authored-by: Yue <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: C++ Component: Gandiva awaiting review Awaiting review labels Nov 14, 2023

niyue commented Nov 14, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 14, 2023

niyue mentioned this pull request Nov 14, 2023

GH-38653: [Packaging][Java][Python][Ruby] Raise the minimum macOS version to 10.15 catalina to allow using new APIs in C++17 #38677

Merged

pitrou requested a review from js8544 November 14, 2023 16:32

This comment was marked as outdated.

Sign in to view

niyue force-pushed the bugfix/io-util branch from cf8d98d to 2f6d6eb Compare November 15, 2023 02:28

Use arrow io util to replace std::filesystem::path in gandiva test_ut…

5ae2764

…il so that AlmaLinux 8 won't complain.

niyue force-pushed the bugfix/io-util branch from 2f6d6eb to 5ae2764 Compare November 15, 2023 05:38

js8544 approved these changes Nov 15, 2023

View reviewed changes

pitrou reviewed Nov 15, 2023

View reviewed changes

cpp/src/gandiva/tests/test_util.cc Outdated Show resolved Hide resolved

niyue and others added 2 commits November 15, 2023 22:44

update test_util.cc

479cae9

Co-authored-by: Antoine Pitrou <[email protected]>

Fix suggestion

7283299

pitrou merged commit 41e45fe into apache:main Nov 15, 2023

pitrou removed the awaiting committer review Awaiting committer review label Nov 15, 2023

Conversation

niyue commented Nov 14, 2023 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Nov 14, 2023

Uh oh!

niyue Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou commented Nov 14, 2023

Uh oh!

pitrou commented Nov 14, 2023

Uh oh!

This comment was marked as outdated.

pitrou commented Nov 14, 2023

Uh oh!

niyue commented Nov 15, 2023

Uh oh!

js8544 commented Nov 15, 2023

Uh oh!

niyue commented Nov 15, 2023

Uh oh!

js8544 left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Nov 15, 2023

Uh oh!

pitrou commented Nov 15, 2023

Uh oh!

github-actions bot commented Nov 15, 2023

Uh oh!

github-actions bot commented Nov 15, 2023

Uh oh!

Uh oh!

pitrou commented Nov 15, 2023

Uh oh!

conbench-apache-arrow bot commented Nov 15, 2023

Uh oh!

niyue commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niyue commented Nov 14, 2023 •

edited by kou

Loading

niyue commented Nov 16, 2023 •

edited

Loading