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

ARROW-17728: [C++][Gandiva] Accept LLVM 15.0 #14125

Merged
merged 7 commits into from
Sep 16, 2022

Conversation

js8544
Copy link
Collaborator

@js8544 js8544 commented Sep 14, 2022

No description provided.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@js8544
Copy link
Collaborator Author

js8544 commented Sep 14, 2022

There is another problem now: LLVM 15 adds dependency on zstd. It looks for target zstd::libzstd_shared, but arrow prefers zstd::libzstd. Let me try to turn off LLVM_ENABLE_ZSTD and see if it works.

@kou kou changed the title ARROW-17728: [C++] Accept llvm 15.0 ARROW-17728: [C++][Gandiva] Accept LLVM 15.0 Sep 14, 2022
@kou
Copy link
Member

kou commented Sep 14, 2022

It seems that the Zstd's official CMake package uses zstd::libzstd_shared and zstd::libzstd_static.
So our CMake configurations should follow it. (And our Findzstd.cmake should support the Zstd's official CMake package too.)

I'll push a change for it to this branch later. Please wait for a while...

@pitrou
Copy link
Member

pitrou commented Sep 15, 2022

There is another problem now: LLVM 15 adds dependency on zstd

Is it a mandatory dependency? zstd shouldn't be required to write a JIT...

@js8544
Copy link
Collaborator Author

js8544 commented Sep 15, 2022

There is another problem now: LLVM 15 adds dependency on zstd

Is it a mandatory dependency? zstd shouldn't be required to write a JIT...

The dependency can be avoided by turning LLVM_ENABLE_ZSTD off. But I'm a CMake newbie and not sure how to do it properly...

@pitrou
Copy link
Member

pitrou commented Sep 15, 2022

Most probably, we must also accomodate for the fact that pre-built binary packages for LLVM probably have zstd enabled anyway.

@kou
Copy link
Member

kou commented Sep 16, 2022

It seems that Gandiva with LLVM 15 have problems at least on MinGW.
But I'm not sure it's caused only on MinGW or can be caused on other platforms.
It's difficult to debug this on MinGW. So I propose that we merge this by disabling Gandiva related tests or Gandiva support only on MinGW for now. We can fix the problem when other platforms provide LLVM 15 and the same problem is occurred on these platforms.

@pitrou
Copy link
Member

pitrou commented Sep 16, 2022

It's difficult to debug this on MinGW. So I propose that we merge this by disabling Gandiva related tests or Gandiva support only on MinGW for now. We can fix the problem when other platforms provide LLVM 15 and the same problem is occurred on these platforms.

That is fine with me.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

We can merge this after we can confirm new CI failures aren't increased with this.

@kou
Copy link
Member

kou commented Sep 16, 2022

"R / rhub/debian-gcc-devel:latest" is failed:

https://github.com/apache/arrow/actions/runs/3063524875/jobs/4951928558#step:5:27585

══ Failed tests ════════════════════════════════════════════════════════════════
── Error (test-duckdb.R:212:3): Joining, auto-cleanup enabled ──────────────────
Error: rapi_execute: Failed to run query
Error: Invalid Error: std::exception
Backtrace:
    ▆
 1. ├─DBI::dbGetQuery(...) at test-duckdb.R:212:2
 2. └─DBI::dbGetQuery(...)
 3.   └─DBI (local) .local(conn, statement, ...)
 4.     ├─DBI::dbSendQuery(conn, statement, ...)
 5.     └─duckdb::dbSendQuery(conn, statement, ...)
 6.       └─duckdb (local) .local(conn, statement, ...)
 7.         └─duckdb:::duckdb_result(...)
 8.           └─duckdb:::duckdb_execute(res)
 9.             └─duckdb:::rapi_execute(...)

[ FAIL 1 | WARN 0 | SKIP 28 | PASS 8467 ]

("AMD64 Ubuntu 20.04 R 4.2 Force-Tests true" has the same failure.)

But this will be solved by #14149. So I merge this.

@kou kou merged commit 9131724 into apache:master Sep 16, 2022
@ursabot
Copy link

ursabot commented Sep 18, 2022

Benchmark runs are scheduled for baseline = 6bc2e01 and contender = 9131724. 9131724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.2% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.28% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.21% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 9131724e ec2-t3-xlarge-us-east-2
[Failed] 9131724e test-mac-arm
[Failed] 9131724e ursa-i9-9960x
[Finished] 9131724e ursa-thinkcentre-m75q
[Finished] 6bc2e010 ec2-t3-xlarge-us-east-2
[Failed] 6bc2e010 test-mac-arm
[Failed] 6bc2e010 ursa-i9-9960x
[Finished] 6bc2e010 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Sep 18, 2022

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
ursa-i9-9960x

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
Lead-authored-by: Jin Shang <shangjin1997@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@js8544 js8544 deleted the jinshang/accept_llvm_15 branch October 11, 2022 08:43
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
Lead-authored-by: Jin Shang <shangjin1997@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants