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.

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

GH-38697: [C++][Gandiva] Use arrow io util to replace std::filesystem::path in gandiva #38698

Merged
merged 3 commits into from
Nov 15, 2023
Merged

GH-38697: [C++][Gandiva] Use arrow io util to replace std::filesystem::path in gandiva #38698

merged 3 commits into from
Nov 15, 2023

Conversation

niyue
Copy link
Contributor

@niyue niyue commented Nov 14, 2023 •
edited by kou

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

@github-actions GitHub Actions
Copy link

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

niyue
niyue commented Nov 14, 2023
View reviewed changes
cpp/src/gandiva/tests/test_util.cc

namespace gandiva {
std::shared_ptr<Configuration> TestConfiguration() {
auto builder = ConfigurationBuilder();
return builder.DefaultConfiguration();
return ConfigurationBuilder::DefaultConfiguration();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 14, 2023
@niyue niyue mentioned this pull request Nov 14, 2023
Merged
@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

This PR tries to fix #38697

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

@pitrou pitrou requested a review from js8544 November 14, 2023 16:32
@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

@github-actions crossbow submit -g cpp

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

@niyue There are Windows failures that need fixing.

@niyue
Copy link
Contributor Author

niyue commented Nov 15, 2023

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

Sure. Updated.

@js8544
Copy link
Collaborator

js8544 commented Nov 15, 2023

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?

@niyue
Copy link
Contributor Author

niyue commented Nov 15, 2023

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
js8544 approved these changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@js8544 js8544 left a comment

Choose a reason for hiding this comment

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

+1. Thanks!

@pitrou
Copy link
Member

pitrou commented Nov 15, 2023

@github-actions crossbow submit -g cpp

@pitrou
Copy link
Member

pitrou commented Nov 15, 2023

@github-actions crossbow submit almalinux

@github-actions GitHub Actions
Copy link

Revision: 5ae2764

Submitted crossbow builds: ursacomputing/crossbow @ actions-fe6233792a

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

@github-actions GitHub Actions
Copy link

Revision: 5ae2764

Submitted crossbow builds: ursacomputing/crossbow @ actions-d28ae2b20a

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

niyue and others added 2 commits November 15, 2023 22:44
@niyue @pitrou
update test_util.cc
479cae9 
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@pitrou
Fix suggestion
7283299
@pitrou
Copy link
Member

pitrou commented Nov 15, 2023

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

@pitrou pitrou merged commit 41e45fe into apache:main Nov 15, 2023
29 of 31 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Nov 15, 2023
@conbench-apache-arrow conbench-apache-arrow
Copy link

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
Copy link
Contributor Author

niyue commented Nov 16, 2023 •
edited

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.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@niyue @pitrou @dgreiss
apacheGH-38697: [C++][Gandiva] Use arrow io util to replace std::file…
75a8881 
…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 <niyue.com@gmail.com>
Co-authored-by: Yue <niyue.com@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou left review comments

@js8544 js8544 js8544 approved these changes

Assignees
No one assigned
Labels
Component: C++ - Gandiva Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++][Gandiva] AlmaLinux 8 CI build failed due to std::filesystem::path usage
3 participants
@niyue @pitrou @js8544