From 18f9149d4aaa92358244382d0a32d982463c1561 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Mon, 17 Nov 2025 14:55:22 -0800 Subject: [PATCH 01/10] Add cpu attrs to llvm code generation. --- cpp/src/gandiva/engine.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc index a55421b1b486..19c2b0260b89 100644 --- a/cpp/src/gandiva/engine.cc +++ b/cpp/src/gandiva/engine.cc @@ -330,6 +330,7 @@ Engine::Engine(const std::shared_ptr& conf, // LLVM 10 doesn't like the expr function name to be the same as the module name auto module_id = "gdv_module_" + std::to_string(reinterpret_cast(this)); module_ = std::make_unique(module_id, *context_); + module_->setDataLayout(target_machine_->createDataLayout()); } Engine::~Engine() {} From 2360631e07aaad26ba4c57e181f206da271fba59 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Mon, 17 Nov 2025 17:28:30 -0800 Subject: [PATCH 02/10] Unit test for cpu attrs. --- cpp/src/gandiva/CMakeLists.txt | 1 + cpp/src/gandiva/target_datalayout_test.cc | 64 +++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 cpp/src/gandiva/target_datalayout_test.cc diff --git a/cpp/src/gandiva/CMakeLists.txt b/cpp/src/gandiva/CMakeLists.txt index e5760243b399..31a86d5da9d6 100644 --- a/cpp/src/gandiva/CMakeLists.txt +++ b/cpp/src/gandiva/CMakeLists.txt @@ -274,6 +274,7 @@ add_gandiva_test(internals-test hash_utils_test.cc gdv_function_stubs_test.cc interval_holder_test.cc + target_datalayout_test.cc tests/test_util.cc EXTRA_LINK_LIBS re2::re2 diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc new file mode 100644 index 000000000000..f5c1c8b9262b --- /dev/null +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -0,0 +1,64 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include +#include +#include + +#include "gandiva/llvm_generator.h" +#include "gandiva/tests/test_util.h" + +namespace gandiva { + +class TestTargetDataLayout : public ::testing::Test { + protected: + void SetUp() override {} +}; + +// Test that verifies the target data layout string representation +// is consistent with the CPU architecture. This test is portable +// across different architectures. +TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { + // Create an LLVM generator with default configuration + ASSERT_OK_AND_ASSIGN(auto generator, LLVMGenerator::Make(TestConfiguration(), false)); + + // Get the module from the generator + llvm::Module* module = generator->module(); + ASSERT_NE(module, nullptr); + + // Get the data layout from the module + const llvm::DataLayout& data_layout = module->getDataLayout(); + std::string data_layout_str = data_layout.getStringRepresentation(); + + // Verify that the data layout string is not empty + EXPECT_FALSE(data_layout_str.empty()); + + // Get the host CPU architecture information + std::string host_cpu = llvm::sys::getHostCPUName().str(); + std::string triple = llvm::sys::getDefaultTargetTriple(); + + // Log the information for debugging + std::cout << "Host CPU: " << host_cpu << std::endl; + std::cout << "Target Triple: " << triple << std::endl; + std::cout << "Data Layout: " << data_layout_str << std::endl; + + // Verify that the data layout string is not empty + EXPECT_FALSE(data_layout_str.empty()); + + } +} // namespace gandiva + From 22d065ce938b6986fa147fbb39a2d248d18dbf6f Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Mon, 17 Nov 2025 17:31:03 -0800 Subject: [PATCH 03/10] Update test --- cpp/src/gandiva/target_datalayout_test.cc | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index f5c1c8b9262b..c2bf1aac9703 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -30,35 +30,24 @@ class TestTargetDataLayout : public ::testing::Test { }; // Test that verifies the target data layout string representation -// is consistent with the CPU architecture. This test is portable -// across different architectures. +// is populated. TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { - // Create an LLVM generator with default configuration ASSERT_OK_AND_ASSIGN(auto generator, LLVMGenerator::Make(TestConfiguration(), false)); - // Get the module from the generator llvm::Module* module = generator->module(); ASSERT_NE(module, nullptr); - // Get the data layout from the module const llvm::DataLayout& data_layout = module->getDataLayout(); std::string data_layout_str = data_layout.getStringRepresentation(); - // Verify that the data layout string is not empty EXPECT_FALSE(data_layout_str.empty()); - // Get the host CPU architecture information std::string host_cpu = llvm::sys::getHostCPUName().str(); std::string triple = llvm::sys::getDefaultTargetTriple(); - // Log the information for debugging std::cout << "Host CPU: " << host_cpu << std::endl; std::cout << "Target Triple: " << triple << std::endl; std::cout << "Data Layout: " << data_layout_str << std::endl; - - // Verify that the data layout string is not empty - EXPECT_FALSE(data_layout_str.empty()); - } } // namespace gandiva From 204cbe56ebeba31f6ea4b9957c1d2bee802c69f7 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Tue, 18 Nov 2025 10:24:32 -0800 Subject: [PATCH 04/10] Remove debug logging from test. --- cpp/src/gandiva/target_datalayout_test.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index c2bf1aac9703..bd67483ec89b 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -17,7 +17,6 @@ #include #include -#include #include "gandiva/llvm_generator.h" #include "gandiva/tests/test_util.h" @@ -41,13 +40,6 @@ TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { std::string data_layout_str = data_layout.getStringRepresentation(); EXPECT_FALSE(data_layout_str.empty()); - - std::string host_cpu = llvm::sys::getHostCPUName().str(); - std::string triple = llvm::sys::getDefaultTargetTriple(); - // Log the information for debugging - std::cout << "Host CPU: " << host_cpu << std::endl; - std::cout << "Target Triple: " << triple << std::endl; - std::cout << "Data Layout: " << data_layout_str << std::endl; } } // namespace gandiva From d1680d21c812655b742775880a180047c47019f2 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Tue, 18 Nov 2025 10:38:16 -0800 Subject: [PATCH 05/10] Remove a space --- cpp/src/gandiva/target_datalayout_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index bd67483ec89b..5e52a7eee3f3 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -40,6 +40,6 @@ TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { std::string data_layout_str = data_layout.getStringRepresentation(); EXPECT_FALSE(data_layout_str.empty()); - } + } } // namespace gandiva From fd0d7e5e81cd2e83fe3c0cb5aa16be7da016b011 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Tue, 18 Nov 2025 10:52:25 -0800 Subject: [PATCH 06/10] Remove a space --- cpp/src/gandiva/target_datalayout_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index 5e52a7eee3f3..58d335cba6de 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -40,6 +40,6 @@ TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { std::string data_layout_str = data_layout.getStringRepresentation(); EXPECT_FALSE(data_layout_str.empty()); - } +} } // namespace gandiva From 704804b525728f85c9c0045ece124ad59d45c486 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Tue, 18 Nov 2025 10:57:43 -0800 Subject: [PATCH 07/10] Remove a space --- cpp/src/gandiva/target_datalayout_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index 58d335cba6de..3124162bbef6 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -42,4 +42,3 @@ TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { EXPECT_FALSE(data_layout_str.empty()); } } // namespace gandiva - From f3d051fdfe59a2a21a6a4ca9b005a578554d1c29 Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Mon, 24 Nov 2025 15:53:58 -0800 Subject: [PATCH 08/10] Updated test class for simplicity. --- cpp/src/gandiva/target_datalayout_test.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/cpp/src/gandiva/target_datalayout_test.cc b/cpp/src/gandiva/target_datalayout_test.cc index 3124162bbef6..0b32c6caf964 100644 --- a/cpp/src/gandiva/target_datalayout_test.cc +++ b/cpp/src/gandiva/target_datalayout_test.cc @@ -23,14 +23,9 @@ namespace gandiva { -class TestTargetDataLayout : public ::testing::Test { - protected: - void SetUp() override {} -}; - // Test that verifies the target data layout string representation // is populated. -TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { +TEST(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { ASSERT_OK_AND_ASSIGN(auto generator, LLVMGenerator::Make(TestConfiguration(), false)); llvm::Module* module = generator->module(); @@ -39,6 +34,6 @@ TEST_F(TestTargetDataLayout, VerifyDataLayoutForArchitecture) { const llvm::DataLayout& data_layout = module->getDataLayout(); std::string data_layout_str = data_layout.getStringRepresentation(); - EXPECT_FALSE(data_layout_str.empty()); + ASSERT_FALSE(data_layout_str.empty()); } } // namespace gandiva From 663774703a5954048515c6d0838e6402d3d770ff Mon Sep 17 00:00:00 2001 From: lriggs Date: Fri, 23 Jan 2026 23:45:50 +0000 Subject: [PATCH 09/10] DX-111455 [Gandiva] Gandiva code is not passing cpu attributes to llvm generator (#106) Fix crash due to decimal alignement --- cpp/src/gandiva/llvm_generator.cc | 19 +- cpp/src/gandiva/tests/CMakeLists.txt | 1 + .../gandiva/tests/decimal_alignment_test.cc | 252 ++++++++++++++++++ 3 files changed, 267 insertions(+), 5 deletions(-) create mode 100644 cpp/src/gandiva/tests/decimal_alignment_test.cc diff --git a/cpp/src/gandiva/llvm_generator.cc b/cpp/src/gandiva/llvm_generator.cc index 4e6480fa1670..0b0bfbc66f8b 100644 --- a/cpp/src/gandiva/llvm_generator.cc +++ b/cpp/src/gandiva/llvm_generator.cc @@ -399,8 +399,13 @@ Status LLVMGenerator::CodeGenExprValue(DexPtr value_expr, int buffer_count, if (output_type_id == arrow::Type::BOOL) { SetPackedBitValue(output_ref, loop_var, output_value->data()); - } else if (arrow::is_primitive(output_type_id) || - output_type_id == arrow::Type::DECIMAL) { + } else if (output_type_id == arrow::Type::DECIMAL) { + // Arrow decimal128 data is only 8-byte aligned, not 16-byte aligned. + // Use CreateAlignedStore with 8-byte alignment to match Arrow's actual alignment. + auto slot_offset = + builder->CreateGEP(types()->IRType(output_type_id), output_ref, loop_var); + builder->CreateAlignedStore(output_value->data(), slot_offset, llvm::MaybeAlign(8)); + } else if (arrow::is_primitive(output_type_id)) { auto slot_offset = builder->CreateGEP(types()->IRType(output_type_id), output_ref, loop_var); builder->CreateStore(output_value->data(), slot_offset); @@ -469,7 +474,7 @@ llvm::Value* LLVMGenerator::GetPackedValidityBitValue(llvm::Value* bitmap, ADD_TRACE("fetch validity bit at position %T", position); llvm::Value* bitmap8 = ir_builder()->CreateBitCast( - bitmap, types()->ptr_type(types()->i8_type()), "bitMapCast"); + bitmap, types()->ptr_type(types()->i8_type()), "bitMapCast"); return AddFunctionCall("bitMapValidityGetBit", types()->i1_type(), {bitmap8, position}); } @@ -602,7 +607,11 @@ void LLVMGenerator::Visitor::Visit(const VectorReadFixedLenValueDex& dex) { case arrow::Type::DECIMAL: { auto slot_offset = builder->CreateGEP(types->i128_type(), slot_ref, slot_index); - slot_value = builder->CreateLoad(types->i128_type(), slot_offset, dex.FieldName()); + // Arrow decimal128 data is only 8-byte aligned, not 16-byte aligned. + // Using CreateLoad with default alignment (16 for i128) causes crashes on misaligned data. + // Use CreateAlignedLoad with 8-byte alignment to match Arrow's actual alignment. + slot_value = builder->CreateAlignedLoad(types->i128_type(), slot_offset, + llvm::MaybeAlign(8), false, dex.FieldName()); lvalue = generator_->BuildDecimalLValue(slot_value, dex.FieldType()); break; } @@ -1108,7 +1117,7 @@ void LLVMGenerator::Visitor::VisitInExpression( const InExprDex& dex_instance = dynamic_cast&>(dex); /* add the holder at the beginning */ - llvm::Constant* ptr_int_cast = + llvm::Constant* ptr_int_cast = types->i64_constant((int64_t)(dex_instance.in_holder().get())); params.push_back(ptr_int_cast); diff --git a/cpp/src/gandiva/tests/CMakeLists.txt b/cpp/src/gandiva/tests/CMakeLists.txt index 68138f50d813..356b976e0058 100644 --- a/cpp/src/gandiva/tests/CMakeLists.txt +++ b/cpp/src/gandiva/tests/CMakeLists.txt @@ -20,6 +20,7 @@ add_gandiva_test(projector-test binary_test.cc boolean_expr_test.cc date_time_test.cc + decimal_alignment_test.cc decimal_single_test.cc decimal_test.cc filter_project_test.cc diff --git a/cpp/src/gandiva/tests/decimal_alignment_test.cc b/cpp/src/gandiva/tests/decimal_alignment_test.cc new file mode 100644 index 000000000000..3bf1bd790213 --- /dev/null +++ b/cpp/src/gandiva/tests/decimal_alignment_test.cc @@ -0,0 +1,252 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// Test for decimal128 alignment issue fix. +// Arrow decimal128 data may be 8-byte aligned but not 16-byte aligned. +// This test verifies that Gandiva handles such data correctly. + +#include + +#include "arrow/memory_pool.h" +#include "arrow/status.h" +#include "arrow/util/decimal.h" +#include "arrow/array/builder_primitive.h" +#include "arrow/array/array_decimal.h" +#include "arrow/buffer.h" + +#include "gandiva/decimal_type_util.h" +#include "gandiva/projector.h" +#include "gandiva/tests/test_util.h" +#include "gandiva/tree_expr_builder.h" + +using arrow::Decimal128; + +namespace gandiva { + +class TestDecimalAlignment : public ::testing::Test { + public: + void SetUp() { pool_ = arrow::default_memory_pool(); } + + protected: + arrow::MemoryPool* pool_; +}; + +// Create a decimal128 array with data at a specific alignment offset +// This simulates the real-world scenario where Arrow data from external sources +// (like JNI/Java) may not be 16-byte aligned. +std::shared_ptr MakeMisalignedDecimalArray( + const std::shared_ptr& type, + const std::vector& values, + int alignment_offset) { + + // Allocate buffer with extra space for misalignment + int64_t data_size = values.size() * 16; // 16 bytes per Decimal128 + int64_t buffer_size = data_size + 16; // Extra space for offset + + std::shared_ptr buffer; + ARROW_EXPECT_OK(arrow::AllocateBuffer(buffer_size).Value(&buffer)); + + // Calculate the starting offset to achieve desired alignment + // We want the data to be 8-byte aligned but NOT 16-byte aligned + uint8_t* raw_data = buffer->mutable_data(); + uintptr_t addr = reinterpret_cast(raw_data); + + // Find offset to get to 8-byte aligned but not 16-byte aligned address + int offset_to_8 = (8 - (addr % 8)) % 8; + int current_16_alignment = (addr + offset_to_8) % 16; + + int final_offset; + if (alignment_offset == 8) { + // Want 8-byte aligned but NOT 16-byte aligned + if (current_16_alignment == 0) { + final_offset = offset_to_8 + 8; // Add 8 to break 16-byte alignment + } else { + final_offset = offset_to_8; + } + } else { + // Want 16-byte aligned + final_offset = (16 - (addr % 16)) % 16; + } + + // Copy decimal values to the offset location + uint8_t* data_start = raw_data + final_offset; + for (size_t i = 0; i < values.size(); i++) { + memcpy(data_start + i * 16, values[i].ToBytes().data(), 16); + } + + // Verify alignment + uintptr_t data_addr = reinterpret_cast(data_start); + EXPECT_EQ(data_addr % 8, 0) << "Data should be 8-byte aligned"; + if (alignment_offset == 8) { + EXPECT_NE(data_addr % 16, 0) << "Data should NOT be 16-byte aligned"; + } + + // Create a sliced buffer starting at our offset + auto sliced_buffer = arrow::SliceBuffer(buffer, final_offset, data_size); + + // Create validity buffer (all valid) + std::shared_ptr validity_buffer; + ARROW_EXPECT_OK(arrow::AllocateBuffer((values.size() + 7) / 8).Value(&validity_buffer)); + memset(validity_buffer->mutable_data(), 0xFF, validity_buffer->size()); + + // Create the array with our misaligned data buffer + auto array_data = arrow::ArrayData::Make( + type, + static_cast(values.size()), + {validity_buffer, sliced_buffer}); + + return std::make_shared(array_data); +} + +// Test that decimal operations work correctly with 8-byte aligned (but not 16-byte aligned) data +TEST_F(TestDecimalAlignment, TestMisalignedDecimalSubtract) { + constexpr int32_t precision = 38; + constexpr int32_t scale = 17; + auto decimal_type = std::make_shared(precision, scale); + auto field_a = arrow::field("a", decimal_type); + auto field_b = arrow::field("b", decimal_type); + auto schema = arrow::schema({field_a, field_b}); + + Decimal128TypePtr output_type; + auto status = DecimalTypeUtil::GetResultType( + DecimalTypeUtil::kOpSubtract, {decimal_type, decimal_type}, &output_type); + ASSERT_OK(status); + + auto res = arrow::field("res", output_type); + auto node_a = TreeExprBuilder::MakeField(field_a); + auto node_b = TreeExprBuilder::MakeField(field_b); + auto subtract = TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); + auto expr = TreeExprBuilder::MakeExpression(subtract, res); + + std::shared_ptr projector; + status = Projector::Make(schema, {expr}, TestConfiguration(), &projector); + ASSERT_OK(status); + + // Create test data + std::vector values_a = {Decimal128(100), Decimal128(200), Decimal128(300)}; + std::vector values_b = {Decimal128(10), Decimal128(20), Decimal128(30)}; + + // Create arrays with 8-byte alignment (but NOT 16-byte aligned) + auto array_a = MakeMisalignedDecimalArray(decimal_type, values_a, 8); + auto array_b = MakeMisalignedDecimalArray(decimal_type, values_b, 8); + + auto in_batch = arrow::RecordBatch::Make(schema, 3, {array_a, array_b}); + + // This should NOT crash even with misaligned data + arrow::ArrayVector outputs; + status = projector->Evaluate(*in_batch, pool_, &outputs); + ASSERT_OK(status); + + // Verify results: 100-10=90, 200-20=180, 300-30=270 + auto result = std::dynamic_pointer_cast(outputs[0]); + ASSERT_NE(result, nullptr); + EXPECT_EQ(result->length(), 3); +} + +// Create a misaligned output buffer for decimal128 +std::shared_ptr MakeMisalignedDecimalOutput( + const std::shared_ptr& type, + int64_t num_records, + int alignment_offset) { + + // Allocate data buffer with extra space for misalignment + int64_t data_size = num_records * 16; // 16 bytes per Decimal128 + int64_t buffer_size = data_size + 16; // Extra space for offset + + std::shared_ptr buffer; + ARROW_EXPECT_OK(arrow::AllocateBuffer(buffer_size).Value(&buffer)); + + uint8_t* raw_data = const_cast(buffer->data()); + uintptr_t addr = reinterpret_cast(raw_data); + + // Find offset to get to 8-byte aligned but not 16-byte aligned address + int offset_to_8 = (8 - (addr % 8)) % 8; + int current_16_alignment = (addr + offset_to_8) % 16; + + int final_offset; + if (alignment_offset == 8) { + if (current_16_alignment == 0) { + final_offset = offset_to_8 + 8; + } else { + final_offset = offset_to_8; + } + } else { + final_offset = (16 - (addr % 16)) % 16; + } + + // Verify alignment + uintptr_t data_addr = reinterpret_cast(raw_data + final_offset); + EXPECT_EQ(data_addr % 8, 0) << "Data should be 8-byte aligned"; + if (alignment_offset == 8) { + EXPECT_NE(data_addr % 16, 0) << "Data should NOT be 16-byte aligned"; + } + + auto sliced_buffer = arrow::SliceBuffer(buffer, final_offset, data_size); + + // Create validity buffer + int64_t bitmap_size = (num_records + 7) / 8; + std::shared_ptr validity_buffer; + ARROW_EXPECT_OK(arrow::AllocateBuffer(bitmap_size).Value(&validity_buffer)); + memset(const_cast(validity_buffer->data()), 0xFF, validity_buffer->size()); + + return arrow::ArrayData::Make(type, num_records, {validity_buffer, sliced_buffer}); +} + +// Test that decimal STORES work correctly with 8-byte aligned (but not 16-byte aligned) output +TEST_F(TestDecimalAlignment, TestMisalignedDecimalStore) { + constexpr int32_t precision = 38; + constexpr int32_t scale = 17; + auto decimal_type = std::make_shared(precision, scale); + auto field_a = arrow::field("a", decimal_type); + auto field_b = arrow::field("b", decimal_type); + auto schema = arrow::schema({field_a, field_b}); + + Decimal128TypePtr output_type; + auto status = DecimalTypeUtil::GetResultType( + DecimalTypeUtil::kOpSubtract, {decimal_type, decimal_type}, &output_type); + ASSERT_OK(status); + + auto res = arrow::field("res", output_type); + auto node_a = TreeExprBuilder::MakeField(field_a); + auto node_b = TreeExprBuilder::MakeField(field_b); + auto subtract = TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); + auto expr = TreeExprBuilder::MakeExpression(subtract, res); + + std::shared_ptr projector; + status = Projector::Make(schema, {expr}, TestConfiguration(), &projector); + ASSERT_OK(status); + + // Create ALIGNED input arrays (using standard Arrow allocation) + auto array_a = MakeArrowArrayDecimal(decimal_type, {Decimal128(100), Decimal128(200), Decimal128(300)}, {true, true, true}); + auto array_b = MakeArrowArrayDecimal(decimal_type, {Decimal128(10), Decimal128(20), Decimal128(30)}, {true, true, true}); + + auto in_batch = arrow::RecordBatch::Make(schema, 3, {array_a, array_b}); + + // Create MISALIGNED output buffer (8-byte aligned but NOT 16-byte aligned) + auto output_data = MakeMisalignedDecimalOutput(output_type, 3, 8); + + // This should NOT crash even with misaligned output buffer + status = projector->Evaluate(*in_batch, {output_data}); + ASSERT_OK(status); + + // Verify the output was written correctly + auto result = std::make_shared(output_data); + EXPECT_EQ(result->length(), 3); +} + +} // namespace gandiva + From 1dcf03c9ef0191640bc493602c1d1cc82dd1213c Mon Sep 17 00:00:00 2001 From: Logan Riggs Date: Fri, 23 Jan 2026 16:40:53 -0800 Subject: [PATCH 10/10] Lint fixes --- cpp/src/gandiva/llvm_generator.cc | 13 ++-- .../gandiva/tests/decimal_alignment_test.cc | 62 +++++++++---------- 2 files changed, 38 insertions(+), 37 deletions(-) diff --git a/cpp/src/gandiva/llvm_generator.cc b/cpp/src/gandiva/llvm_generator.cc index 0b0bfbc66f8b..0f0918b3a1c7 100644 --- a/cpp/src/gandiva/llvm_generator.cc +++ b/cpp/src/gandiva/llvm_generator.cc @@ -474,7 +474,7 @@ llvm::Value* LLVMGenerator::GetPackedValidityBitValue(llvm::Value* bitmap, ADD_TRACE("fetch validity bit at position %T", position); llvm::Value* bitmap8 = ir_builder()->CreateBitCast( - bitmap, types()->ptr_type(types()->i8_type()), "bitMapCast"); + bitmap, types()->ptr_type(types()->i8_type()), "bitMapCast"); return AddFunctionCall("bitMapValidityGetBit", types()->i1_type(), {bitmap8, position}); } @@ -608,10 +608,11 @@ void LLVMGenerator::Visitor::Visit(const VectorReadFixedLenValueDex& dex) { case arrow::Type::DECIMAL: { auto slot_offset = builder->CreateGEP(types->i128_type(), slot_ref, slot_index); // Arrow decimal128 data is only 8-byte aligned, not 16-byte aligned. - // Using CreateLoad with default alignment (16 for i128) causes crashes on misaligned data. - // Use CreateAlignedLoad with 8-byte alignment to match Arrow's actual alignment. - slot_value = builder->CreateAlignedLoad(types->i128_type(), slot_offset, - llvm::MaybeAlign(8), false, dex.FieldName()); + // Using CreateLoad with default alignment (16 for i128) causes crashes on + // misaligned data. Use CreateAlignedLoad with 8-byte alignment to match Arrow's + // actual alignment. + slot_value = builder->CreateAlignedLoad( + types->i128_type(), slot_offset, llvm::MaybeAlign(8), false, dex.FieldName()); lvalue = generator_->BuildDecimalLValue(slot_value, dex.FieldType()); break; } @@ -1117,7 +1118,7 @@ void LLVMGenerator::Visitor::VisitInExpression( const InExprDex& dex_instance = dynamic_cast&>(dex); /* add the holder at the beginning */ - llvm::Constant* ptr_int_cast = + llvm::Constant* ptr_int_cast = types->i64_constant((int64_t)(dex_instance.in_holder().get())); params.push_back(ptr_int_cast); diff --git a/cpp/src/gandiva/tests/decimal_alignment_test.cc b/cpp/src/gandiva/tests/decimal_alignment_test.cc index 3bf1bd790213..3028ec81f314 100644 --- a/cpp/src/gandiva/tests/decimal_alignment_test.cc +++ b/cpp/src/gandiva/tests/decimal_alignment_test.cc @@ -21,12 +21,12 @@ #include +#include "arrow/array/array_decimal.h" +#include "arrow/array/builder_primitive.h" +#include "arrow/buffer.h" #include "arrow/memory_pool.h" #include "arrow/status.h" #include "arrow/util/decimal.h" -#include "arrow/array/builder_primitive.h" -#include "arrow/array/array_decimal.h" -#include "arrow/buffer.h" #include "gandiva/decimal_type_util.h" #include "gandiva/projector.h" @@ -50,25 +50,23 @@ class TestDecimalAlignment : public ::testing::Test { // (like JNI/Java) may not be 16-byte aligned. std::shared_ptr MakeMisalignedDecimalArray( const std::shared_ptr& type, - const std::vector& values, - int alignment_offset) { - + const std::vector& values, int alignment_offset) { // Allocate buffer with extra space for misalignment int64_t data_size = values.size() * 16; // 16 bytes per Decimal128 int64_t buffer_size = data_size + 16; // Extra space for offset - + std::shared_ptr buffer; ARROW_EXPECT_OK(arrow::AllocateBuffer(buffer_size).Value(&buffer)); - + // Calculate the starting offset to achieve desired alignment // We want the data to be 8-byte aligned but NOT 16-byte aligned uint8_t* raw_data = buffer->mutable_data(); uintptr_t addr = reinterpret_cast(raw_data); - + // Find offset to get to 8-byte aligned but not 16-byte aligned address int offset_to_8 = (8 - (addr % 8)) % 8; int current_16_alignment = (addr + offset_to_8) % 16; - + int final_offset; if (alignment_offset == 8) { // Want 8-byte aligned but NOT 16-byte aligned @@ -81,38 +79,37 @@ std::shared_ptr MakeMisalignedDecimalArray( // Want 16-byte aligned final_offset = (16 - (addr % 16)) % 16; } - + // Copy decimal values to the offset location uint8_t* data_start = raw_data + final_offset; for (size_t i = 0; i < values.size(); i++) { memcpy(data_start + i * 16, values[i].ToBytes().data(), 16); } - + // Verify alignment uintptr_t data_addr = reinterpret_cast(data_start); EXPECT_EQ(data_addr % 8, 0) << "Data should be 8-byte aligned"; if (alignment_offset == 8) { EXPECT_NE(data_addr % 16, 0) << "Data should NOT be 16-byte aligned"; } - + // Create a sliced buffer starting at our offset auto sliced_buffer = arrow::SliceBuffer(buffer, final_offset, data_size); - + // Create validity buffer (all valid) std::shared_ptr validity_buffer; ARROW_EXPECT_OK(arrow::AllocateBuffer((values.size() + 7) / 8).Value(&validity_buffer)); memset(validity_buffer->mutable_data(), 0xFF, validity_buffer->size()); - + // Create the array with our misaligned data buffer - auto array_data = arrow::ArrayData::Make( - type, - static_cast(values.size()), - {validity_buffer, sliced_buffer}); - + auto array_data = arrow::ArrayData::Make(type, static_cast(values.size()), + {validity_buffer, sliced_buffer}); + return std::make_shared(array_data); } -// Test that decimal operations work correctly with 8-byte aligned (but not 16-byte aligned) data +// Test that decimal operations work correctly with 8-byte aligned (but not 16-byte +// aligned) data TEST_F(TestDecimalAlignment, TestMisalignedDecimalSubtract) { constexpr int32_t precision = 38; constexpr int32_t scale = 17; @@ -129,7 +126,8 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalSubtract) { auto res = arrow::field("res", output_type); auto node_a = TreeExprBuilder::MakeField(field_a); auto node_b = TreeExprBuilder::MakeField(field_b); - auto subtract = TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); + auto subtract = + TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); auto expr = TreeExprBuilder::MakeExpression(subtract, res); std::shared_ptr projector; @@ -139,7 +137,7 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalSubtract) { // Create test data std::vector values_a = {Decimal128(100), Decimal128(200), Decimal128(300)}; std::vector values_b = {Decimal128(10), Decimal128(20), Decimal128(30)}; - + // Create arrays with 8-byte alignment (but NOT 16-byte aligned) auto array_a = MakeMisalignedDecimalArray(decimal_type, values_a, 8); auto array_b = MakeMisalignedDecimalArray(decimal_type, values_b, 8); @@ -159,10 +157,8 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalSubtract) { // Create a misaligned output buffer for decimal128 std::shared_ptr MakeMisalignedDecimalOutput( - const std::shared_ptr& type, - int64_t num_records, + const std::shared_ptr& type, int64_t num_records, int alignment_offset) { - // Allocate data buffer with extra space for misalignment int64_t data_size = num_records * 16; // 16 bytes per Decimal128 int64_t buffer_size = data_size + 16; // Extra space for offset @@ -206,7 +202,8 @@ std::shared_ptr MakeMisalignedDecimalOutput( return arrow::ArrayData::Make(type, num_records, {validity_buffer, sliced_buffer}); } -// Test that decimal STORES work correctly with 8-byte aligned (but not 16-byte aligned) output +// Test that decimal STORES work correctly with 8-byte aligned (but not 16-byte aligned) +// output TEST_F(TestDecimalAlignment, TestMisalignedDecimalStore) { constexpr int32_t precision = 38; constexpr int32_t scale = 17; @@ -223,7 +220,8 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalStore) { auto res = arrow::field("res", output_type); auto node_a = TreeExprBuilder::MakeField(field_a); auto node_b = TreeExprBuilder::MakeField(field_b); - auto subtract = TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); + auto subtract = + TreeExprBuilder::MakeFunction("subtract", {node_a, node_b}, output_type); auto expr = TreeExprBuilder::MakeExpression(subtract, res); std::shared_ptr projector; @@ -231,8 +229,11 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalStore) { ASSERT_OK(status); // Create ALIGNED input arrays (using standard Arrow allocation) - auto array_a = MakeArrowArrayDecimal(decimal_type, {Decimal128(100), Decimal128(200), Decimal128(300)}, {true, true, true}); - auto array_b = MakeArrowArrayDecimal(decimal_type, {Decimal128(10), Decimal128(20), Decimal128(30)}, {true, true, true}); + auto array_a = MakeArrowArrayDecimal( + decimal_type, {Decimal128(100), Decimal128(200), Decimal128(300)}, + {true, true, true}); + auto array_b = MakeArrowArrayDecimal( + decimal_type, {Decimal128(10), Decimal128(20), Decimal128(30)}, {true, true, true}); auto in_batch = arrow::RecordBatch::Make(schema, 3, {array_a, array_b}); @@ -249,4 +250,3 @@ TEST_F(TestDecimalAlignment, TestMisalignedDecimalStore) { } } // namespace gandiva -