GH-51245: [C++][Compute][Gandiva] Add support for LLVM 23.1 by HuaHuaY · Pull Request #51266 · apache/arrow

HuaHuaY · 2026-09-09T14:11:09Z

Rationale for this change

Add support for LLVM 23.1.

What changes are included in this PR?

Remove static from some template functions in header files.
Fixed a bug in CastImpl template instantiation about Date and Timestamp data types, and add a unit test.
Remove the build target attributes in bitcode generated by Gandiva.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

GitHub Issue: [C++][Gandiva] Add support for LLVM 23.1 #51245

Copilot

🟡 Changes recommended

The LLVM>=23 JITLink lambda in engine.cc does not capture memory_manager, which should fail to compile.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Arrow C++/Gandiva compatibility updates for LLVM 23.1, including JITLink integration adjustments, fixing scalar cast template instantiation for date/timestamp types, and ensuring generated/embedded bitcode is host-appropriate at runtime.

Changes:

Add LLVM 23.1 to supported versions and relax Gandiva conda env LLVM pinning.
Update Gandiva JITLink configuration and strip build-target function attributes from embedded bitcode before JIT-compiling.
Fix CastImpl template constraints for date/timestamp casts and add targeted unit tests; remove static from several header template helpers to avoid LLVM/Clang-related linkage issues.

File summaries

File	Description
cpp/src/gandiva/engine.cc	Adjust JITLink layer creation for LLVM 23+ and remove target CPU/features attrs from embedded bitcode before linking.
cpp/src/arrow/util/future.h	Remove `static` from a header template helper to avoid problematic linkage/instantiation behavior.
cpp/src/arrow/util/bit_block_counter.h	Remove `static` from header template visitors.
cpp/src/arrow/util/async_generator.h	Remove `static` from header template generator factories.
cpp/src/arrow/scalar.cc	Fix date/timestamp cast template constraints to match type-based dispatch.
cpp/src/arrow/scalar_test.cc	Add unit tests covering Date32/Date64/Timestamp `CastTo` paths.
cpp/src/arrow/compute/kernels/vector_hash.cc	Attempt to suppress unused warnings in a templated observer overload.
cpp/src/arrow/compute/kernels/codegen_internal.h	Remove `static` from header template helpers used in kernel codegen.
cpp/src/arrow/compute/function_internal.h	Remove `static` from a header template specialization for list scalar conversion.
cpp/src/arrow/compute/exec.cc	Remove an unused internal helper template (no longer referenced).
cpp/CMakeLists.txt	Add LLVM 23.1 to `ARROW_LLVM_VERSIONS`.
ci/conda_env_gandiva.txt	Unpin `llvmdev` to allow LLVM 23.x in the Gandiva conda environment.

Review details

Files reviewed: 12/12 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

HuaHuaY

To assist the reviewer with the review process, I submit some explanatory notes.

HuaHuaY · 2026-09-09T14:13:52Z

 template <typename T, typename VisitFunc, typename NullFunc>
  requires std::is_void_v<std::invoke_result_t<VisitFunc, typename GetViewType<T>::T>>
-static void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,
+void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,


LLVM 23 added -Wunused-template to -Wall.
https://releases.llvm.org/23.1.0/tools/clang/docs/ReleaseNotes.html

-Wunused-template is now part of -Wunused (which is enabled by -Wall). It diagnoses unused function and variable templates with internal linkage, which in a header is a latent ODR hazard. It can be disabled with -Wno-unused-template. (#202945)

I don't think there's any harm in removing static. Template functions are inherently similar to inline functions, and ODR issues won't arise.

HuaHuaY · 2026-09-09T14:14:29Z

  const FunctionOptions* options_;
 };

-template <typename ExecutorType,


It's a template function which doesn't have any callers.

HuaHuaY · 2026-09-09T14:16:02Z

 // date to date
 template <typename To>
-enable_if_t<std::is_same<To, Date64Scalar>::value, Result<std::shared_ptr<Scalar>>>
+enable_if_t<std::is_same<To, Date64Type>::value, Result<std::shared_ptr<Scalar>>>


This is another issue exposed by -Wunused-template. The template instantiation type was incorrect here, causing the function that was supposed to execute this path to fall through to the "NotImplemented" exception instead. A unit test has been added.

HuaHuaY · 2026-09-09T14:17:43Z

 arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
                               const std::string& error_context) {
  if (!expected) {
+    // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM builds


This comment was supposed to be moved here from line 425 during a previous code refactoring, but it was overlooked.

HuaHuaY · 2026-09-09T14:27:07Z

  if (maybe_use_jit_link.ok()) {
    ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
-#  if LLVM_VERSION_MAJOR >= 21
+#  if LLVM_VERSION_MAJOR >= 23


llvm/llvm-project#192214 added one more parameter.

HuaHuaY · 2026-09-09T14:29:04Z

+
+  // Built-in bitcode is JIT-compiled on the runtime host. Do not retain the target
+  // selected by Clang when the bitcode was built.
+  RemoveBuildTargetAttributes(*src_ir_module);


The requirements for inlining have become stricter in LLVM 23.
https://releases.llvm.org/23.1.0/docs/ReleaseNotes.html#changes-to-the-llvm-ir

alwaysinline no longer bypasses inlining compatibility checks based on target features. Inlining will only be performed if it is safe to do so.

Copilot

🟡 Changes recommended

There are a few concrete build/CI and warning-suppression issues (typoed helper name, incorrect [[maybe_unused]] placement, and unbounded LLVM conda dependency) that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

cpp/src/gandiva/engine.cc:135

The NOTE mentions llvm::handleAllErrors() but this function doesn't call it directly (it calls llvm::toString(...)). Rewording avoids confusion about what is actually being avoided here.
cpp/src/gandiva/engine.cc:204
Typo in helper name: CreateMemmoryManager should be CreateMemoryManager (double 'm' in Memory). This is easy to miss and will propagate to the call site below.

This issue also appears on line 218 of the same file.

cpp/src/gandiva/engine.cc:222

Call site should match the corrected helper name (CreateMemoryManager).

        });
#  else
    ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
#    if LLVM_VERSION_MAJOR >= 21
    jit_builder.setObjectLinkingLayerCreator([&](llvm::orc::ExecutionSession& ES) {

cpp/src/arrow/compute/kernels/vector_hash.cc:141

[[maybe_unused]] is applied to the function here, but the unused entity is the parameter index. If -Wunused-parameter is enabled, this won't suppress the warning. Prefer applying the attribute to the parameter (or use ARROW_UNUSED(index) in the body).


  template <class Index>
  [[maybe_unused]] void ObserveNullNotFound(Index index) {
    ARROW_LOG(FATAL) << "ObserveNullNotFound without err_status should not be called";
  }

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

The changes are consistent with the stated LLVM 23.1 support goal, are localized, and include targeted unit coverage for the fixed scalar casting behavior.

Review details

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟡 Changes recommended

There are correctness/build issues in the updated code paths (notably unused-warning suppression and external bitcode handling consistency) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cpp/src/arrow/compute/kernels/vector_hash.cc:140

[[maybe_unused]] is applied to the function, but the warning being addressed is the unused parameter index. This doesn’t suppress -Wunused-parameter (and may not fix the build with -Werror). Apply the attribute to the parameter instead (or explicitly ignore it).

  template <class Index>
  [[maybe_unused]] void ObserveNullNotFound(Index index) {
    ARROW_LOG(FATAL) << "ObserveNullNotFound without err_status should not be called";

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

pitrou · 2026-09-09T15:57:02Z

    auto llvm_memory_buffer_ref = AsLLVMMemoryBuffer(*buffer);
    auto module_or_error = llvm::parseBitcodeFile(llvm_memory_buffer_ref, *context());
-    ARROW_RETURN_NOT_OK(VerifyAndLinkModule(*module_, std::move(module_or_error)));
+    ARROW_ASSIGN_OR_RAISE(
+        auto src_ir_module,
+        AsArrowResult(module_or_error, "Failed to verify and link module: "));
+    ARROW_RETURN_NOT_OK(VerifyAndLinkModule(*module_, std::move(src_ir_module)));


This comment by Copilot is wrong, isn't it? Or should we actually force the precompiled bitcode's build target attributes to the same ones as the function registry?

pitrou

LGTM except for the latest Copilot comment which I'm not sure about

HuaHuaY · 2026-09-09T16:04:14Z

LGTM except for the latest Copilot comment which I'm not sure about

I'm also not sure about that whether we should modify the build target attributes of external precompiled bitcode. But even if we disregard this Copilot recommendation, the only difference lies in whether LLVM decides to inline the code or not. It does not fail our unit tests.

pitrou · 2026-09-09T16:05:16Z

Right. Perhaps we can just ping some Gandiva maintainer so that they later run benchmarks and see if anything needs fixing.

pitrou · 2026-09-09T16:12:11Z

Well, some Gandiva tests have failed on the Conda C++ CI build (which uses LLVM 23.1)...

HuaHuaY · 2026-09-09T16:25:59Z

Well, some Gandiva tests have failed on the Conda C++ CI build (which uses LLVM 23.1)...

I couldn't reproduce the issue with simply running the tests in my local environment (macOS, debug mode). I'll look into the CI failures tomorrow.

add support for llvm 23.1

7a63d79

HuaHuaY requested a review from pitrou as a code owner September 9, 2026 14:11

Copilot AI lite review requested due to automatic review settings September 9, 2026 14:11

HuaHuaY requested review from assignUser, jonkeane, kou and raulcd as code owners September 9, 2026 14:11

github-actions Bot added the awaiting review Awaiting review label Sep 9, 2026

Copilot started reviewing on behalf of HuaHuaY September 9, 2026 14:17 View session

github-actions Bot added Component: C++ Component: Gandiva labels Sep 9, 2026

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread cpp/src/gandiva/engine.cc Outdated

Comment thread cpp/src/arrow/compute/kernels/vector_hash.cc

HuaHuaY commented Sep 9, 2026

View reviewed changes

pitrou reviewed Sep 9, 2026

View reviewed changes

Comment thread cpp/src/gandiva/engine.cc Outdated

Comment thread cpp/src/gandiva/engine.cc

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 9, 2026

fix cpplint ci and add a minor refactor

658e96e

Copilot AI review requested due to automatic review settings September 9, 2026 14:47

Copilot started reviewing on behalf of HuaHuaY September 9, 2026 15:01 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread ci/conda_env_gandiva.txt Outdated

Update llvmdev version constraint in conda environment

eee5f51

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 9, 2026 15:09

Copilot started reviewing on behalf of HuaHuaY September 9, 2026 15:16 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

add more comments

6c10961

Copilot AI review requested due to automatic review settings September 9, 2026 15:32

Copilot started reviewing on behalf of HuaHuaY September 9, 2026 15:35 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

pitrou approved these changes Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

HuaHuaY commented Sep 9, 2026 • edited 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!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

HuaHuaY left a comment

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

pitrou Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

HuaHuaY commented Sep 9, 2026

Uh oh!

pitrou commented Sep 9, 2026

Uh oh!

pitrou commented Sep 9, 2026

Uh oh!

HuaHuaY commented Sep 9, 2026 • 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

HuaHuaY commented Sep 9, 2026 •

edited

Loading

HuaHuaY commented Sep 9, 2026 •

edited

Loading