Skip to content

Navigation Menu

Sign in
Sign up

GH-51245: [C++][Compute][Gandiva] Add support for LLVM 23.1 #51266

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

Open
HuaHuaY wants to merge 9 commits into apache:main
base: main
Choose a base branch
Loading
from HuaHuaY:llvm_23
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some comments aren't visible on the classic Files Changed page.

2 changes: 1 addition & 1 deletion ci/conda_env_gandiva.txt
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@
# under the License.

clang
llvmdev<23
llvmdev<24
1 change: 1 addition & 0 deletions cpp/CMakeLists.txt
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ set(ARROW_DOC_DIR "${CMAKE_INSTALL_DOCDIR}")
set(BUILD_SUPPORT_DIR "${CMAKE_SOURCE_DIR}/build-support")

set(ARROW_LLVM_VERSIONS
"23.1"
"22.1"
"21.1"
"20.1"
Expand Down
10 changes: 0 additions & 10 deletions cpp/src/arrow/compute/exec.cc
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,16 +1185,6 @@ class ScalarAggExecutor : public KernelExecutorImpl<ScalarAggregateKernel> {
const FunctionOptions* options_;
};

template <typename ExecutorType,

@HuaHuaY HuaHuaY Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

typename FunctionType = typename ExecutorType::FunctionType>
Result<std::unique_ptr<KernelExecutor>> MakeExecutor(ExecContext* ctx,
const Function* func,
const FunctionOptions* options) {
DCHECK_EQ(ExecutorType::function_kind, func->kind());
auto typed_func = checked_cast<const FunctionType*>(func);
return std::make_unique<ExecutorType>(ctx, typed_func, options);
}

} // namespace

Status PropagateNulls(KernelContext* ctx, const ExecSpan& batch, ArrayData* output) {
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/arrow/compute/function_internal.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,8 @@ static inline std::enable_if_t<is_optional_v<T>, Result<T>> GenericFromScalar(
}

template <typename T>
static enable_if_same<typename CTypeTraits<T>::ArrowType, ListType, Result<T>>
GenericFromScalar(const std::shared_ptr<Scalar>& value) {
enable_if_same<typename CTypeTraits<T>::ArrowType, ListType, Result<T>> GenericFromScalar(
const std::shared_ptr<Scalar>& value) {
using ValueType = typename T::value_type;
if (value->type->id() != Type::LIST) {
return Status::Invalid("Expected type LIST but got ", value->type->ToString());
Expand Down
14 changes: 7 additions & 7 deletions cpp/src/arrow/compute/kernels/codegen_internal.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,8 @@ struct UnboxScalar<Decimal256Type> {

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,
NullFunc&& null_func) {
void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,

@HuaHuaY HuaHuaY Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

pitrou reacted with thumbs up emoji
NullFunc&& null_func) {
VisitArraySpanInline<T>(
arr,
[&](typename GetViewType<T>::PhysicalType v) {
Expand All @@ -494,8 +494,8 @@ static void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,
template <typename T, typename VisitFunc, typename NullFunc>
requires std::is_same_v<std::invoke_result_t<VisitFunc, typename GetViewType<T>::T>,
Status>
static Status VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,
NullFunc&& null_func) {
Status VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,
NullFunc&& null_func) {
return VisitArraySpanInline<T>(
arr,
[&](typename GetViewType<T>::PhysicalType v) {
Expand All @@ -507,8 +507,8 @@ static Status VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_fun
// Like VisitArrayValuesInline, but for binary functions.

template <typename Arg0Type, typename Arg1Type, typename VisitFunc, typename NullFunc>
static void VisitTwoArrayValuesInline(const ArraySpan& arr0, const ArraySpan& arr1,
VisitFunc&& valid_func, NullFunc&& null_func) {
void VisitTwoArrayValuesInline(const ArraySpan& arr0, const ArraySpan& arr1,
VisitFunc&& valid_func, NullFunc&& null_func) {
ArrayIterator<Arg0Type> arr0_it(arr0);
ArrayIterator<Arg1Type> arr1_it(arr1);

Expand Down Expand Up @@ -584,7 +584,7 @@ namespace applicator {
// static Status Call(KernelContext*, const Scalar& arg0, const ArraySpan& arg1,
// ExecResult* out)
template <typename Operator>
static Status SimpleBinary(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
Status SimpleBinary(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
if (batch.length == 0) return Status::OK();

if (batch[0].is_array()) {
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/compute/kernels/vector_hash.cc
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class ValueCountsAction final : ActionBase {
}

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

Expand Down
10 changes: 5 additions & 5 deletions cpp/src/arrow/scalar.cc
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,21 +1191,21 @@ constexpr int64_t kMillisecondsInDay = 86400000;

// 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>>>

@HuaHuaY HuaHuaY Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

CastImpl(const Date32Scalar& from, std::shared_ptr<DataType> to_type) {
return std::make_shared<Date64Scalar>(from.value * kMillisecondsInDay,
std::move(to_type));
}
template <typename To>
enable_if_t<std::is_same<To, Date32Scalar>::value, Result<std::shared_ptr<Scalar>>>
enable_if_t<std::is_same<To, Date32Type>::value, Result<std::shared_ptr<Scalar>>>
CastImpl(const Date64Scalar& from, std::shared_ptr<DataType> to_type) {
return std::make_shared<Date32Scalar>(
static_cast<int32_t>(from.value / kMillisecondsInDay), std::move(to_type));
}

// timestamp 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>>>
CastImpl(const TimestampScalar& from, std::shared_ptr<DataType> to_type) {
ARROW_ASSIGN_OR_RAISE(
auto millis,
Expand All @@ -1214,7 +1214,7 @@ CastImpl(const TimestampScalar& from, std::shared_ptr<DataType> to_type) {
std::move(to_type));
}
template <typename To>
enable_if_t<std::is_same<To, Date32Scalar>::value, Result<std::shared_ptr<Scalar>>>
enable_if_t<std::is_same<To, Date32Type>::value, Result<std::shared_ptr<Scalar>>>
CastImpl(const TimestampScalar& from, std::shared_ptr<DataType> to_type) {
ARROW_ASSIGN_OR_RAISE(
auto millis,
Expand All @@ -1225,7 +1225,7 @@ CastImpl(const TimestampScalar& from, std::shared_ptr<DataType> to_type) {

// date to timestamp
template <typename To, typename From>
enable_if_timestamp<Result<std::shared_ptr<To>>> CastImpl(
enable_if_timestamp<To, Result<std::shared_ptr<Scalar>>> CastImpl(
const DateScalar<From>& from, std::shared_ptr<DataType> to_type) {
using ToScalar = typename TypeTraits<To>::ScalarType;
int64_t millis = from.value;
Expand Down
33 changes: 33 additions & 0 deletions cpp/src/arrow/scalar_test.cc
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,39 @@ TEST(TestDateScalars, MakeScalar) {
Date64Scalar(-188171LL * 24 * 60 * 60 * 1000));
}

TEST(TestDateScalars, CastTo) {
constexpr int64_t kMillisecondsInDay = 86400000;

ASSERT_OK_AND_ASSIGN(auto casted_date64, Date32Scalar(2).CastTo(date64()));
EXPECT_EQ(*casted_date64, Date64Scalar(2 * kMillisecondsInDay));

ASSERT_OK_AND_ASSIGN(auto casted_date32,
Date64Scalar(2 * kMillisecondsInDay).CastTo(date32()));
EXPECT_EQ(*casted_date32, Date32Scalar(2));

const auto timestamp_type = timestamp(TimeUnit::SECOND);

ASSERT_OK_AND_ASSIGN(auto timestamp_from_date32,
Date32Scalar(2).CastTo(timestamp_type));
EXPECT_EQ(*timestamp_from_date32, TimestampScalar(2 * 24 * 60 * 60, timestamp_type));

ASSERT_OK_AND_ASSIGN(auto timestamp_from_date64,
Date64Scalar(2 * kMillisecondsInDay).CastTo(timestamp_type));
EXPECT_EQ(*timestamp_from_date64, TimestampScalar(2 * 24 * 60 * 60, timestamp_type));

ASSERT_OK_AND_ASSIGN(
auto date64_from_timestamp,
TimestampScalar(2 * kMillisecondsInDay + 3, timestamp(TimeUnit::MILLI))
.CastTo(date64()));
EXPECT_EQ(*date64_from_timestamp, Date64Scalar(2 * kMillisecondsInDay));

ASSERT_OK_AND_ASSIGN(
auto date32_from_timestamp,
TimestampScalar(2 * kMillisecondsInDay + 3, timestamp(TimeUnit::MILLI))
.CastTo(date32()));
EXPECT_EQ(*date32_from_timestamp, Date32Scalar(2));
}

TEST(TestTimeScalars, Basics) {
auto type1 = time32(TimeUnit::MILLI);
auto type2 = time32(TimeUnit::SECOND);
Expand Down
7 changes: 3 additions & 4 deletions cpp/src/arrow/util/async_generator.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1863,7 +1863,7 @@ constexpr int kDefaultBackgroundQRestart = 16;
///
/// This generator will queue up to max_q blocks
template <typename T>
static Result<AsyncGenerator<T>> MakeBackgroundGenerator(
Result<AsyncGenerator<T>> MakeBackgroundGenerator(
Iterator<T> iterator, internal::Executor* io_executor,
int max_q = kDefaultBackgroundMaxQ, int q_restart = kDefaultBackgroundQRestart) {
if (max_q < q_restart) {
Expand All @@ -1887,15 +1887,14 @@ static Result<AsyncGenerator<T>> MakeBackgroundGenerator(
///
/// This generator does not queue
template <typename T>
static Result<AsyncGenerator<T>> MakeBlockingGenerator(
std::shared_ptr<Iterator<T>> iterator) {
Result<AsyncGenerator<T>> MakeBlockingGenerator(std::shared_ptr<Iterator<T>> iterator) {
return [it = std::move(iterator)]() mutable -> Future<T> {
return Future<T>::MakeFinished(it->Next());
};
}

template <typename T>
static Result<AsyncGenerator<T>> MakeBlockingGenerator(Iterator<T> iterator) {
Result<AsyncGenerator<T>> MakeBlockingGenerator(Iterator<T> iterator) {
return MakeBlockingGenerator(std::make_shared<Iterator<T>>(std::move(iterator)));
}

Expand Down
24 changes: 12 additions & 12 deletions cpp/src/arrow/util/bit_block_counter.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,8 @@ class ARROW_EXPORT OptionalBinaryBitBlockCounter {
// Functional-style bit block visitors.

template <typename VisitNotNull, typename VisitNull>
static Status VisitBitBlocks(const uint8_t* bitmap, int64_t offset, int64_t length,
VisitNotNull&& visit_not_null, VisitNull&& visit_null) {
Status VisitBitBlocks(const uint8_t* bitmap, int64_t offset, int64_t length,
VisitNotNull&& visit_not_null, VisitNull&& visit_null) {
internal::OptionalBitBlockCounter bit_counter(bitmap, offset, length);
int64_t position = 0;
while (position < length) {
Expand All @@ -453,8 +453,8 @@ static Status VisitBitBlocks(const uint8_t* bitmap, int64_t offset, int64_t leng
}

template <typename VisitNotNull, typename VisitNull>
static void VisitBitBlocksVoid(const uint8_t* bitmap, int64_t offset, int64_t length,
VisitNotNull&& visit_not_null, VisitNull&& visit_null) {
void VisitBitBlocksVoid(const uint8_t* bitmap, int64_t offset, int64_t length,
VisitNotNull&& visit_not_null, VisitNull&& visit_null) {
internal::OptionalBitBlockCounter bit_counter(bitmap, offset, length);
int64_t position = 0;
while (position < length) {
Expand All @@ -480,10 +480,10 @@ static void VisitBitBlocksVoid(const uint8_t* bitmap, int64_t offset, int64_t le
}

template <typename VisitNotNull, typename VisitNull>
static Status VisitTwoBitBlocks(const uint8_t* left_bitmap, int64_t left_offset,
const uint8_t* right_bitmap, int64_t right_offset,
int64_t length, VisitNotNull&& visit_not_null,
VisitNull&& visit_null) {
Status VisitTwoBitBlocks(const uint8_t* left_bitmap, int64_t left_offset,
const uint8_t* right_bitmap, int64_t right_offset,
int64_t length, VisitNotNull&& visit_not_null,
VisitNull&& visit_null) {
if (left_bitmap == NULLPTR || right_bitmap == NULLPTR) {
// At most one bitmap is present
if (left_bitmap == NULLPTR) {
Expand Down Expand Up @@ -524,10 +524,10 @@ static Status VisitTwoBitBlocks(const uint8_t* left_bitmap, int64_t left_offset,
}

template <typename VisitNotNull, typename VisitNull>
static void VisitTwoBitBlocksVoid(const uint8_t* left_bitmap, int64_t left_offset,
const uint8_t* right_bitmap, int64_t right_offset,
int64_t length, VisitNotNull&& visit_not_null,
VisitNull&& visit_null) {
void VisitTwoBitBlocksVoid(const uint8_t* left_bitmap, int64_t left_offset,
const uint8_t* right_bitmap, int64_t right_offset,
int64_t length, VisitNotNull&& visit_not_null,
VisitNull&& visit_null) {
if (left_bitmap == NULLPTR || right_bitmap == NULLPTR) {
// At most one bitmap is present
if (left_bitmap == NULLPTR) {
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/util/future.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ class WeakFuture {
/// If a Result<Future> holds an error instead of a Future, construct a finished Future
/// holding that error.
template <typename T>
static Future<T> DeferNotOk(Result<Future<T>> maybe_future) {
Future<T> DeferNotOk(Result<Future<T>> maybe_future) {
if (ARROW_PREDICT_FALSE(!maybe_future.ok())) {
return Future<T>::MakeFinished(std::move(maybe_future).status());
}
Expand Down
14 changes: 10 additions & 4 deletions cpp/src/gandiva/decimal_ir.cc
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "arrow/util/logging_internal.h"
#include "gandiva/decimal_ir.h"
#include "gandiva/decimal_type_util.h"
#include "gandiva/llvm_util_internal.h"

// Algorithms adapted from Apache Impala

Expand Down Expand Up @@ -424,9 +425,15 @@ Status DecimalIR::BuildCompare(const std::string& function_name,
llvm::Value* DecimalIR::CallDecimalFunction(const std::string& function_name,
llvm::Type* return_type,
const std::vector<llvm::Value*>& params) {
auto create_call = [&](const std::vector<llvm::Value*>& args) {
auto* call = ir_builder()->CreateCall(module()->getFunction(function_name), args);
internal::CopyZExtAttrs(*call->getCalledFunction(), *call);
return call;
};

if (kDecimalIRBuilderFunctions.count(function_name) != 0) {
// this is fn built with the irbuilder.
return ir_builder()->CreateCall(module()->getFunction(function_name), params);
return create_call(params);
}

// ppre-compiler fn : disassemble i128 to two i64s and re-assemble.
Expand Down Expand Up @@ -454,7 +461,7 @@ llvm::Value* DecimalIR::CallDecimalFunction(const std::string& function_name,
dis_assembled_args.push_back(out_low_ptr);

// Make call to pre-compiled IR function.
ir_builder()->CreateCall(module()->getFunction(function_name), dis_assembled_args);
create_call(dis_assembled_args);

auto out_high = ir_builder()->CreateLoad(i64, out_high_ptr);
auto out_low = ir_builder()->CreateLoad(i64, out_low_ptr);
Expand All @@ -463,8 +470,7 @@ llvm::Value* DecimalIR::CallDecimalFunction(const std::string& function_name,
DCHECK_NE(return_type, types()->void_type());

// Make call to pre-compiled IR function.
result = ir_builder()->CreateCall(module()->getFunction(function_name),
dis_assembled_args);
result = create_call(dis_assembled_args);
}
return result;
}
Expand Down
Loading
Loading

AltStyle によって変換されたページ (->オリジナル) /