From b8e89eccce593549da9e84806dca7d611cc41c9e Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 14 Sep 2026 15:56:36 +0800 Subject: [PATCH 1/5] perf(read): skip the payload Concatenate when late materialization matches a single run --- .../multi_literals_leaf_function.cpp | 13 +- .../null_false_leaf_binary_function.cpp | 11 +- .../late_materializing_file_batch_reader.cpp | 15 ++- ...e_materializing_file_batch_reader_test.cpp | 74 +++++++++++ src/paimon/common/utils/arrow/arrow_utils.cpp | 82 ++++++++++++ src/paimon/common/utils/arrow/arrow_utils.h | 16 +++ .../common/utils/arrow/arrow_utils_test.cpp | 122 ++++++++++++++++++ 7 files changed, 315 insertions(+), 18 deletions(-) diff --git a/src/paimon/common/predicate/multi_literals_leaf_function.cpp b/src/paimon/common/predicate/multi_literals_leaf_function.cpp index c8a41812d..d423e57c0 100644 --- a/src/paimon/common/predicate/multi_literals_leaf_function.cpp +++ b/src/paimon/common/predicate/multi_literals_leaf_function.cpp @@ -31,6 +31,7 @@ #include "arrow/memory_pool.h" #include "arrow/type.h" #include "paimon/common/predicate/literal_converter.h" +#include "paimon/common/utils/arrow/arrow_utils.h" #include "paimon/common/utils/arrow/status_utils.h" #include "paimon/common/utils/checked_cast.h" #include "paimon/data/decimal.h" @@ -201,15 +202,9 @@ Result> ProbeInValueSet(const arrow::Array& array, const arrow // last owner at the end of the statement and leave that reference dangling. std::shared_ptr matches_array = matches.make_array(); const auto& matched = checked_cast(*matches_array); - std::vector is_valid(matched.length(), 0); - for (int64_t i = 0; i < matched.length(); i++) { - if (matched.IsNull(i)) { - // `IN` and `NOT IN` are both false on a null value, leave the row at 0. - continue; - } - is_valid[i] = static_cast(matched.Value(i) != negate); - } - return is_valid; + // `IN` and `NOT IN` are both false on a null value, so the null rows unpack to 0 rather than to + // the negation of a value they do not hold. + return ArrowUtils::UnpackBooleansToBytes(matched, negate); } } // namespace diff --git a/src/paimon/common/predicate/null_false_leaf_binary_function.cpp b/src/paimon/common/predicate/null_false_leaf_binary_function.cpp index a53c06101..5968ca285 100644 --- a/src/paimon/common/predicate/null_false_leaf_binary_function.cpp +++ b/src/paimon/common/predicate/null_false_leaf_binary_function.cpp @@ -32,6 +32,7 @@ #include "arrow/scalar.h" #include "arrow/type.h" #include "paimon/common/predicate/literal_converter.h" +#include "paimon/common/utils/arrow/arrow_utils.h" #include "paimon/common/utils/arrow/status_utils.h" #include "paimon/common/utils/checked_cast.h" #include "paimon/common/utils/field_type_utils.h" @@ -196,13 +197,9 @@ Result> ProbeComparison(const arrow::Array& array, const char* // last owner at the end of the statement and leave that reference dangling. std::shared_ptr matches_array = matches.make_array(); const auto& matched = checked_cast(*matches_array); - std::vector is_valid(matched.length(), 0); - for (int64_t i = 0; i < matched.length(); i++) { - // A kernel emits a null for a null row, and every `NullFalseLeafBinaryFunction` is false on - // one, so a null row is left at 0 rather than read as a value. - is_valid[i] = static_cast(!matched.IsNull(i) && matched.Value(i)); - } - return is_valid; + // A kernel emits a null for a null row, and every `NullFalseLeafBinaryFunction` is false on + // one, so the null rows unpack to 0 rather than to a value they do not hold. + return ArrowUtils::UnpackBooleansToBytes(matched, /*negate=*/false); } } // namespace diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp index a5984c703..5554b92b2 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp @@ -193,8 +193,19 @@ Result LateMaterializingFileBatchReader::ReadPayload // Compact the payload superset down to the matched rows (ascending file row order). PAIMON_ASSIGN_OR_RAISE(arrow::ArrayVector payload_slices, ReaderUtils::GenerateFilteredArrayVector(payload_array, valid)); - PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr payload_compacted, - arrow::Concatenate(payload_slices, arrow_pool_.get())); + std::shared_ptr payload_compacted; + if (payload_slices.size() == 1) { + // A single run has nothing to merge, and `AssembleFullBatch` normalizes each column + // anyway, so hand the slice over rather than let `Concatenate` copy the payload a + // second time. A run at the batch head then stays zero-copy; a mid-batch run is still + // copied once by that normalization, so this is never worse. Where it stays zero-copy + // the slice keeps the batch's buffers alive until the consumer releases the assembled + // batch, bounded by the batch size times the prefetch queue depth. + payload_compacted = std::move(payload_slices.front()); + } else { + PAIMON_ASSIGN_OR_RAISE_FROM_ARROW( + payload_compacted, arrow::Concatenate(payload_slices, arrow_pool_.get())); + } auto card = static_cast(valid.Cardinality()); if (probe_cursor_ + card > probe_data_->length()) { diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp index 4346eeed7..d4759d8c4 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp @@ -29,6 +29,7 @@ #include #include "arrow/api.h" +#include "arrow/array/builder_dict.h" #include "arrow/array/builder_nested.h" #include "arrow/c/bridge.h" #include "gtest/gtest.h" @@ -214,6 +215,27 @@ class LateMaterializingFileBatchReaderTest : public ::testing::Test { return array; } + // Build a struct with a dictionary-encoded payload column [k:int64, v:dictionary]. + std::shared_ptr BuildDictionaryPayloadData(int32_t n) { + auto type = + arrow::struct_({arrow::field("k", arrow::int64()), + arrow::field("v", arrow::dictionary(arrow::int32(), arrow::utf8()))}); + arrow::StructBuilder builder(type, arrow::default_memory_pool(), + {std::make_shared(), + std::make_shared()}); + auto* k = checked_cast(builder.field_builder(0)); + auto* v = checked_cast(builder.field_builder(1)); + for (int32_t i = 0; i < n; ++i) { + EXPECT_TRUE(builder.Append().ok()); + EXPECT_TRUE(k->Append(i).ok()); + // Repeat a few values so the dictionary is shorter than the column it encodes. + EXPECT_TRUE(v->Append("d_" + std::to_string(i % 3)).ok()); + } + std::shared_ptr array; + EXPECT_TRUE(builder.Finish(&array).ok()); + return array; + } + protected: std::shared_ptr pool_; std::shared_ptr k_field_; @@ -533,6 +555,58 @@ TEST_F(LateMaterializingFileBatchReaderTest, NestedPayloadColumn) { } } +// A payload column that the inner reader hands over dictionary-encoded has to stay that way in the +// output. Compacting a batch down to a single matched run passes the run straight to the assembly +// step rather than concatenating it, so on that path the encoding survives only because nothing +// rewrites the array. Both shapes of a lone run are covered: one starting at the batch head, which +// reaches the output untouched, and one starting mid-batch, which the per-column offset +// normalization still copies. +TEST_F(LateMaterializingFileBatchReaderTest, DictionaryPayloadColumnKeepsEncoding) { + auto data = BuildDictionaryPayloadData(8); + auto type = data->type(); + auto mock = std::make_unique(data, type, /*batch_size=*/3); + ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( + std::move(mock), GetArrowPool(pool_))); + // probe = {k}; payload = {v}. k >= 5 matches only row 5 of the batch holding rows 3..5, a run + // at batch offset 2, and both rows of the batch holding rows 6..7, a run at batch offset 0. + auto predicate = + PredicateBuilder::GreaterOrEqual(/*field_index=*/0, "k", FieldType::BIGINT, Literal(5l)); + ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(type->fields()), predicate, std::nullopt)); + + std::vector> rows; + while (true) { + ASSERT_OK_AND_ASSIGN(BatchReader::ReadBatchWithBitmap batch_with_bitmap, + reader->NextBatchWithBitmap()); + if (BatchReader::IsEofBatch(batch_with_bitmap)) { + break; + } + auto& [batch, bitmap] = batch_with_bitmap; + auto& [c_array, c_schema] = batch; + arrow::Result> imported = + arrow::ImportArray(c_array.get(), c_schema.get()); + ASSERT_TRUE(imported.ok()) << imported.status().ToString(); + auto struct_array = arrow::internal::checked_pointer_cast(*imported); + auto k = arrow::internal::checked_pointer_cast( + struct_array->GetFieldByName("k")); + std::shared_ptr v = struct_array->GetFieldByName("v"); + ASSERT_TRUE(k && v); + ASSERT_EQ(v->type_id(), arrow::Type::DICTIONARY); + auto dict = arrow::internal::checked_pointer_cast(v); + auto indices = arrow::internal::checked_pointer_cast(dict->indices()); + auto values = arrow::internal::checked_pointer_cast(dict->dictionary()); + ASSERT_TRUE(indices && values); + for (int64_t i = 0; i < struct_array->length(); ++i) { + rows.emplace_back(k->Value(i), values->GetString(indices->Value(i))); + } + } + ASSERT_EQ(rows.size(), 3u); // k = 5,6,7 + for (size_t i = 0; i < rows.size(); ++i) { + int64_t expected_k = 5 + static_cast(i); + EXPECT_EQ(rows[i].first, expected_k); + EXPECT_EQ(rows[i].second, "d_" + std::to_string(expected_k % 3)); + } +} + // The late-materialization reader must work correctly as an inner reader driven by // PrefetchFileBatchReaderImpl (schema broadcast, range dispatch, seek, row-id tracking). TEST_F(LateMaterializingFileBatchReaderTest, WorksAsInnerOfPrefetchReader) { diff --git a/src/paimon/common/utils/arrow/arrow_utils.cpp b/src/paimon/common/utils/arrow/arrow_utils.cpp index 45884f5bb..08a087268 100644 --- a/src/paimon/common/utils/arrow/arrow_utils.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils.cpp @@ -19,9 +19,16 @@ #include "paimon/common/utils/arrow/arrow_utils.h" +#include +#include +#include +#include + #include "arrow/array/array_base.h" #include "arrow/array/array_nested.h" +#include "arrow/array/array_primitive.h" #include "arrow/array/concatenate.h" +#include "arrow/array/data.h" #include "arrow/array/util.h" #include "arrow/buffer.h" #include "arrow/c/abi.h" @@ -334,6 +341,31 @@ Result> RebaseToZeroOffset( return CopyToZeroOffset(data, pool); } +/// The eight bytes one byte of a bitmap expands to: `kExpandedBytes[b].bytes[i]` is bit `i` of `b`. +/// Naming the bytes one at a time rather than building a word keeps the expansion independent of +/// the host's byte order. +struct ExpandedByte { + char bytes[8]; +}; + +constexpr ExpandedByte ExpandBitmapByte(uint8_t bits) { + ExpandedByte expanded{}; + for (int i = 0; i < 8; i++) { + expanded.bytes[i] = static_cast((bits >> i) & uint8_t{1}); + } + return expanded; +} + +constexpr std::array MakeExpandedBytes() { + std::array table{}; + for (int bits = 0; bits < 256; bits++) { + table[static_cast(bits)] = ExpandBitmapByte(static_cast(bits)); + } + return table; +} + +constexpr std::array kExpandedBytes = MakeExpandedBytes(); + } // namespace const char* ArrowUtils::kArrowSchemaMetadataKey = "ARROW:schema"; @@ -432,6 +464,56 @@ uint64_t ArrowUtils::GetArrayMemoryUsage(const std::shared_ptr return result; } +std::vector ArrowUtils::UnpackBooleansToBytes(const arrow::BooleanArray& array, bool negate) { + const int64_t length = array.length(); + std::vector is_valid(static_cast(length)); + if (length == 0) { + return is_valid; + } + + const std::shared_ptr& data = array.data(); + // `BooleanArray::Value(i)` and `Array::IsValid(i)` both read their bitmap at bit `i + offset` + // from the start of the buffer, so one bit index serves the value and the validity alike. + const int64_t bit_offset = data->offset; + const uint8_t* values = data->GetValuesSafe(/*i=*/1, /*absolute_offset=*/0); + const uint8_t* validity = array.null_bitmap_data(); + // Without a validity bitmap `Array::IsValid` falls back to `null_count != length`, which makes + // every row valid unless the array is null throughout. One that is has no row left to unpack, + // and is the only array whose value bitmap is not read. + if (validity == nullptr && data->null_count == length) { + return is_valid; + } + + auto unpack_row = [&](int64_t row) -> char { + const int64_t bit = bit_offset + row; + const bool holds_value = validity == nullptr || arrow::bit_util::GetBit(validity, bit); + return static_cast(holds_value && (arrow::bit_util::GetBit(values, bit) != negate)); + }; + + char* out = is_valid.data(); + int64_t row = 0; + // The rows sharing the leading partial byte, which the byte at a time body cannot take whole. + // None when the bit offset is already on a byte boundary, as it is for the batch a kernel has + // just written. + const int64_t head = std::min(length, (8 - (bit_offset & 7)) & 7); + for (; row < head; row++) { + out[row] = unpack_row(row); + } + for (; row + 8 <= length; row += 8) { + const int64_t byte = (bit_offset + row) >> 3; + // Every row holds a value when there is no validity bitmap, so all eight bits pass. + const uint8_t valid_byte = validity == nullptr ? uint8_t{0xFF} : validity[byte]; + const uint8_t value_byte = values[byte]; + const uint8_t bits = negate ? static_cast(~value_byte & valid_byte) + : static_cast(value_byte & valid_byte); + std::memcpy(out + row, kExpandedBytes[bits].bytes, sizeof(ExpandedByte)); + } + for (; row < length; row++) { + out[row] = unpack_row(row); + } + return is_valid; +} + bool ArrowUtils::EqualsIgnoreNullable(const std::shared_ptr& type, const std::shared_ptr& other_type) { if (type->id() != other_type->id() || type->num_fields() != other_type->num_fields()) { diff --git a/src/paimon/common/utils/arrow/arrow_utils.h b/src/paimon/common/utils/arrow/arrow_utils.h index f78135073..2a213155f 100644 --- a/src/paimon/common/utils/arrow/arrow_utils.h +++ b/src/paimon/common/utils/arrow/arrow_utils.h @@ -53,6 +53,22 @@ class PAIMON_EXPORT ArrowUtils { static uint64_t GetArrayMemoryUsage(const std::shared_ptr& data); + /// Expands the booleans of `array` into one byte per row, the shape `LeafFunction::Test` + /// returns. Row `i` becomes 1 when it holds a value and that value, once `negate` has been + /// applied to it, is true; a null row becomes 0. + /// + /// A kernel a leaf function probes with writes a boolean bitmap, one bit per row, so the rows + /// it selects have to be spread over a byte each before the caller can use them. Reading that + /// bitmap a byte at a time and writing eight rows at once keeps the spread off the per row + /// path, where reading it a bit at a time costs a shift, a mask and a byte store per row. + /// + /// @param array The bitmap to expand. Its offset and its validity are honoured exactly as + /// `BooleanArray::Value()` and `Array::IsValid()` honour them. + /// @param negate Whether to take the rows whose bit is clear rather than the rows whose bit is + /// set, which is what `NOT IN` needs. A null row stays 0 either way. + /// @return One byte per row of `array`. + static std::vector UnpackBooleansToBytes(const arrow::BooleanArray& array, bool negate); + static Result> RemoveFieldFromStructArray( const std::shared_ptr& struct_array, const std::string& field_name); diff --git a/src/paimon/common/utils/arrow/arrow_utils_test.cpp b/src/paimon/common/utils/arrow/arrow_utils_test.cpp index bc95a6d68..92da42420 100644 --- a/src/paimon/common/utils/arrow/arrow_utils_test.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils_test.cpp @@ -19,6 +19,8 @@ #include "paimon/common/utils/arrow/arrow_utils.h" +#include +#include #include #include #include @@ -1215,4 +1217,124 @@ TEST(ArrowUtilsTest, TestFlattenUnresolvableDictionaries) { } } +namespace { + +/// What `ArrowUtils::UnpackBooleansToBytes` has to agree with: every row taken one at a time +/// through the very accessors the helper replaces. +std::vector UnpackRowByRow(const arrow::BooleanArray& array, bool negate) { + std::vector is_valid(static_cast(array.length()), 0); + for (int64_t i = 0; i < array.length(); i++) { + if (array.IsNull(i)) { + continue; + } + is_valid[i] = static_cast(array.Value(i) != negate); + } + return is_valid; +} + +/// A boolean array of `values.size()` rows holding the value `values[i]` at row `i`, with a +/// validity bitmap that marks row `i` as holding one when `valid[i]` is true. +std::shared_ptr BooleansOf(const std::vector& values, + const std::vector& valid) { + const int64_t length = static_cast(values.size()); + std::shared_ptr value_bits = arrow::AllocateBitmap(length).ValueOrDie(); + std::shared_ptr valid_bits = arrow::AllocateBitmap(length).ValueOrDie(); + int64_t null_count = 0; + for (int64_t i = 0; i < length; i++) { + arrow::bit_util::SetBitTo(value_bits->mutable_data(), i, values[static_cast(i)]); + arrow::bit_util::SetBitTo(valid_bits->mutable_data(), i, valid[static_cast(i)]); + null_count += valid[static_cast(i)] ? 0 : 1; + } + return std::make_shared(length, value_bits, valid_bits, null_count); +} + +} // namespace + +TEST(ArrowUtilsTest, TestUnpackBooleansToBytes) { + std::shared_ptr made = + arrow::ipc::internal::json::ArrayFromJSON(arrow::boolean(), R"([true, false, null, true])") + .ValueOrDie(); + const auto& array = checked_cast(*made); + + // A null row is left at 0 whatever the value bitmap holds for it. + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/false), + std::vector({1, 0, 0, 1})); + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/true), + std::vector({0, 1, 0, 0})); +} + +TEST(ArrowUtilsTest, TestUnpackBooleansToBytesAgreesWithTheAccessorsOnEverySlice) { + // Neither the row count nor the periods the values and the nulls repeat on are a multiple of + // eight, so the slices below land on a partial leading byte, on whole bytes and on a partial + // trailing one, whichever offset and length they take. + constexpr int64_t kRows = 41; + std::vector values; + std::vector valid; + for (int64_t i = 0; i < kRows; i++) { + values.push_back((i * 7 + 3) % 5 != 0); + valid.push_back(i % 4 != 0); + } + const std::shared_ptr source = BooleansOf(values, valid); + + for (int64_t offset = 0; offset <= kRows; offset++) { + for (int64_t length = 0; length <= kRows - offset; length++) { + // `Slice` returns a new owner, so it has to be held for as long as the slice is read. + std::shared_ptr sliced = source->Slice(offset, length); + const auto& slice = checked_cast(*sliced); + for (bool negate : {false, true}) { + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(slice, negate), + UnpackRowByRow(slice, negate)) + << "offset=" << offset << " length=" << length << " negate=" << negate; + } + } + } +} + +TEST(ArrowUtilsTest, TestUnpackBooleansToBytesWithoutAValidityBitmap) { + // A bitmap written for a column with no null in it carries no validity buffer at all, which + // `Array::IsValid` reads as every row holding a value. + const int64_t length = 10; + std::shared_ptr value_bits = arrow::AllocateBitmap(length).ValueOrDie(); + for (int64_t i = 0; i < length; i++) { + arrow::bit_util::SetBitTo(value_bits->mutable_data(), i, i % 3 != 0); + } + const arrow::BooleanArray array(length, value_bits, /*null_bitmap=*/nullptr, + /*null_count=*/0); + + std::vector expected(static_cast(length)); + for (int64_t i = 0; i < length; i++) { + expected[static_cast(i)] = static_cast(i % 3 != 0); + } + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/false), expected); + ASSERT_EQ(UnpackRowByRow(array, /*negate=*/false), expected); + + // `negate` still applies once every row holds a value. + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/true), + UnpackRowByRow(array, /*negate=*/true)); +} + +TEST(ArrowUtilsTest, TestUnpackBooleansToBytesOfAnArrayThatIsNullThroughout) { + // Without a validity bitmap `Array::IsValid` falls back to `null_count != length`, so an array + // whose null count is its length holds no value at any row, however its value bitmap reads. + const int64_t length = 9; + std::shared_ptr value_bits = arrow::AllocateBitmap(length).ValueOrDie(); + arrow::bit_util::SetBitsTo(value_bits->mutable_data(), /*offset=*/0, length, /*bits=*/true); + const arrow::BooleanArray array(length, value_bits, /*null_bitmap=*/nullptr, + /*null_count=*/length); + + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/false), + std::vector(static_cast(length), 0)); + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/true), + std::vector(static_cast(length), 0)); + ASSERT_EQ(UnpackRowByRow(array, /*negate=*/true), + std::vector(static_cast(length), 0)); +} + +TEST(ArrowUtilsTest, TestUnpackBooleansToBytesOfAnEmptyArray) { + const arrow::BooleanArray array(/*length=*/0, /*data=*/nullptr); + + ASSERT_TRUE(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/false).empty()); + ASSERT_TRUE(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/true).empty()); +} + } // namespace paimon::test From 63f2b3cc87b937f110bac69172e04c9d5dbe557e Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 14 Sep 2026 17:25:45 +0800 Subject: [PATCH 2/5] fix --- src/paimon/common/utils/arrow/arrow_utils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/paimon/common/utils/arrow/arrow_utils.cpp b/src/paimon/common/utils/arrow/arrow_utils.cpp index 08a087268..ac4dcef75 100644 --- a/src/paimon/common/utils/arrow/arrow_utils.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils.cpp @@ -350,7 +350,7 @@ struct ExpandedByte { constexpr ExpandedByte ExpandBitmapByte(uint8_t bits) { ExpandedByte expanded{}; - for (int i = 0; i < 8; i++) { + for (int32_t i = 0; i < 8; i++) { expanded.bytes[i] = static_cast((bits >> i) & uint8_t{1}); } return expanded; @@ -358,7 +358,7 @@ constexpr ExpandedByte ExpandBitmapByte(uint8_t bits) { constexpr std::array MakeExpandedBytes() { std::array table{}; - for (int bits = 0; bits < 256; bits++) { + for (int32_t bits = 0; bits < 256; bits++) { table[static_cast(bits)] = ExpandBitmapByte(static_cast(bits)); } return table; From 7d4ee71b5bff906f038aafe1a47413fcb1a5224c Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 14 Sep 2026 17:41:51 +0800 Subject: [PATCH 3/5] fix --- .../late_materializing_file_batch_reader_test.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp index d4759d8c4..0849836ca 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp @@ -220,11 +220,15 @@ class LateMaterializingFileBatchReaderTest : public ::testing::Test { auto type = arrow::struct_({arrow::field("k", arrow::int64()), arrow::field("v", arrow::dictionary(arrow::int32(), arrow::utf8()))}); + // `StringDictionaryBuilder` picks the narrowest index type its dictionary fits in - int8 + // for the three values below - and `StructBuilder::type()` takes each field type from its + // builder, so the column would come out as `dictionary` whatever `type` says. + // The 32 builder is what keeps the index width this fixture declares. arrow::StructBuilder builder(type, arrow::default_memory_pool(), {std::make_shared(), - std::make_shared()}); + std::make_shared()}); auto* k = checked_cast(builder.field_builder(0)); - auto* v = checked_cast(builder.field_builder(1)); + auto* v = checked_cast(builder.field_builder(1)); for (int32_t i = 0; i < n; ++i) { EXPECT_TRUE(builder.Append().ok()); EXPECT_TRUE(k->Append(i).ok()); @@ -590,7 +594,10 @@ TEST_F(LateMaterializingFileBatchReaderTest, DictionaryPayloadColumnKeepsEncodin struct_array->GetFieldByName("k")); std::shared_ptr v = struct_array->GetFieldByName("v"); ASSERT_TRUE(k && v); - ASSERT_EQ(v->type_id(), arrow::Type::DICTIONARY); + // The index width is checked before the casts below, which a release build resolves + // statically and would otherwise read the index buffer at the wrong width. + ASSERT_TRUE(v->type()->Equals(*arrow::dictionary(arrow::int32(), arrow::utf8()))) + << v->type()->ToString(); auto dict = arrow::internal::checked_pointer_cast(v); auto indices = arrow::internal::checked_pointer_cast(dict->indices()); auto values = arrow::internal::checked_pointer_cast(dict->dictionary()); From 23ac888857a8b76bebc5ffa2eb170d87a31136f0 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 09:59:04 +0800 Subject: [PATCH 4/5] fix --- src/paimon/common/utils/arrow/arrow_utils.cpp | 2 +- src/paimon/common/utils/arrow/arrow_utils_test.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/paimon/common/utils/arrow/arrow_utils.cpp b/src/paimon/common/utils/arrow/arrow_utils.cpp index ac4dcef75..dcb4769d0 100644 --- a/src/paimon/common/utils/arrow/arrow_utils.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils.cpp @@ -475,7 +475,7 @@ std::vector ArrowUtils::UnpackBooleansToBytes(const arrow::BooleanArray& a // `BooleanArray::Value(i)` and `Array::IsValid(i)` both read their bitmap at bit `i + offset` // from the start of the buffer, so one bit index serves the value and the validity alike. const int64_t bit_offset = data->offset; - const uint8_t* values = data->GetValuesSafe(/*i=*/1, /*absolute_offset=*/0); + const auto values = data->GetValuesSafe(/*i=*/1, /*absolute_offset=*/0); const uint8_t* validity = array.null_bitmap_data(); // Without a validity bitmap `Array::IsValid` falls back to `null_count != length`, which makes // every row valid unless the array is null throughout. One that is has no row left to unpack, diff --git a/src/paimon/common/utils/arrow/arrow_utils_test.cpp b/src/paimon/common/utils/arrow/arrow_utils_test.cpp index 92da42420..929c8d83c 100644 --- a/src/paimon/common/utils/arrow/arrow_utils_test.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils_test.cpp @@ -1236,7 +1236,7 @@ std::vector UnpackRowByRow(const arrow::BooleanArray& array, bool negate) /// validity bitmap that marks row `i` as holding one when `valid[i]` is true. std::shared_ptr BooleansOf(const std::vector& values, const std::vector& valid) { - const int64_t length = static_cast(values.size()); + const auto length = static_cast(values.size()); std::shared_ptr value_bits = arrow::AllocateBitmap(length).ValueOrDie(); std::shared_ptr valid_bits = arrow::AllocateBitmap(length).ValueOrDie(); int64_t null_count = 0; From da9478f3871e03e04530723ed4742490f5e2c0d0 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 11:47:09 +0800 Subject: [PATCH 5/5] fix --- src/paimon/common/utils/arrow/arrow_utils.cpp | 6 ------ .../common/utils/arrow/arrow_utils_test.cpp | 19 ++++++++++--------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/paimon/common/utils/arrow/arrow_utils.cpp b/src/paimon/common/utils/arrow/arrow_utils.cpp index dcb4769d0..5a0222403 100644 --- a/src/paimon/common/utils/arrow/arrow_utils.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils.cpp @@ -477,12 +477,6 @@ std::vector ArrowUtils::UnpackBooleansToBytes(const arrow::BooleanArray& a const int64_t bit_offset = data->offset; const auto values = data->GetValuesSafe(/*i=*/1, /*absolute_offset=*/0); const uint8_t* validity = array.null_bitmap_data(); - // Without a validity bitmap `Array::IsValid` falls back to `null_count != length`, which makes - // every row valid unless the array is null throughout. One that is has no row left to unpack, - // and is the only array whose value bitmap is not read. - if (validity == nullptr && data->null_count == length) { - return is_valid; - } auto unpack_row = [&](int64_t row) -> char { const int64_t bit = bit_offset + row; diff --git a/src/paimon/common/utils/arrow/arrow_utils_test.cpp b/src/paimon/common/utils/arrow/arrow_utils_test.cpp index 929c8d83c..ea3fd1e37 100644 --- a/src/paimon/common/utils/arrow/arrow_utils_test.cpp +++ b/src/paimon/common/utils/arrow/arrow_utils_test.cpp @@ -1314,19 +1314,20 @@ TEST(ArrowUtilsTest, TestUnpackBooleansToBytesWithoutAValidityBitmap) { } TEST(ArrowUtilsTest, TestUnpackBooleansToBytesOfAnArrayThatIsNullThroughout) { - // Without a validity bitmap `Array::IsValid` falls back to `null_count != length`, so an array - // whose null count is its length holds no value at any row, however its value bitmap reads. + // A predicate over a column no row holds a value in leaves every row null. The value bitmap is + // all set, so a 1 among the unpacked bytes can only be a null row read as holding a value. const int64_t length = 9; - std::shared_ptr value_bits = arrow::AllocateBitmap(length).ValueOrDie(); - arrow::bit_util::SetBitsTo(value_bits->mutable_data(), /*offset=*/0, length, /*bits=*/true); - const arrow::BooleanArray array(length, value_bits, /*null_bitmap=*/nullptr, - /*null_count=*/length); + const std::shared_ptr array = + BooleansOf(std::vector(static_cast(length), true), + std::vector(static_cast(length), false)); + const arrow::Status validated = array->ValidateFull(); + ASSERT_TRUE(validated.ok()) << validated.ToString(); - ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/false), + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(*array, /*negate=*/false), std::vector(static_cast(length), 0)); - ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(array, /*negate=*/true), + ASSERT_EQ(ArrowUtils::UnpackBooleansToBytes(*array, /*negate=*/true), std::vector(static_cast(length), 0)); - ASSERT_EQ(UnpackRowByRow(array, /*negate=*/true), + ASSERT_EQ(UnpackRowByRow(*array, /*negate=*/true), std::vector(static_cast(length), 0)); }