Skip to content

Commit 632d7f4

Browse files
authored
fix(reader): ensure batch readers return zero-offset Arrow arrays (alibaba#440)
1 parent a099fee commit 632d7f4

8 files changed

Lines changed: 164 additions & 11 deletions

File tree

include/paimon/reader/batch_reader.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ class PAIMON_EXPORT BatchReader {
4040
/// If EOF is reached, returns an OK status with a nullptr array. Returns an error status only
4141
/// for critical failures (e.g., IO errors). Once an error is returned, this method must not be
4242
/// retried, as it will repeatedly return the same error code.
43+
/// \note IMPORTANT: A non-EOF ArrowArray and all its nested child arrays must have offset 0 to
44+
/// avoid potential issues during conversion through the Arrow C Data Interface.
4345
///
4446
/// @return A result containing a `::ReadBatch`, which consists of a unique pointer to
4547
/// `ArrowArray` and a unique pointer to `ArrowSchema`. Returned array contains a `_VALUE_KIND`
@@ -52,6 +54,8 @@ class PAIMON_EXPORT BatchReader {
5254
/// If EOF is reached, returns an OK status with a nullptr array. Returns an error status only
5355
/// for critical failures (e.g., IO errors). Once an error is returned, this method must not be
5456
/// retried, as it will repeatedly return the same error code.
57+
/// \note IMPORTANT: A non-EOF ArrowArray and all its nested child arrays must have offset 0 to
58+
/// avoid potential issues during conversion through the Arrow C Data Interface.
5559
///
5660
/// @return A result containing a `::ReadBatch` and a valid bitmap. `::ReadBatch` consists of a
5761
/// unique pointer to `ArrowArray` and a unique pointer to `ArrowSchema`. Returned array

src/paimon/common/utils/arrow/arrow_utils.cpp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,30 @@
1818

1919
#include "arrow/array/array_base.h"
2020
#include "arrow/array/array_nested.h"
21+
#include "arrow/array/concatenate.h"
2122
#include "arrow/util/compression.h"
2223
#include "fmt/format.h"
2324
#include "paimon/common/utils/arrow/status_utils.h"
2425
#include "paimon/common/utils/string_utils.h"
2526

2627
namespace paimon {
2728

29+
namespace {
30+
31+
bool HasNonZeroOffset(const std::shared_ptr<arrow::ArrayData>& data) {
32+
if (data->offset != 0) {
33+
return true;
34+
}
35+
for (const auto& child : data->child_data) {
36+
if (HasNonZeroOffset(child)) {
37+
return true;
38+
}
39+
}
40+
return false;
41+
}
42+
43+
} // namespace
44+
2845
const char* ArrowUtils::kArrowSchemaMetadataKey = "ARROW:schema";
2946

3047
Result<std::shared_ptr<arrow::Schema>> ArrowUtils::DataTypeToSchema(
@@ -167,6 +184,28 @@ Result<std::shared_ptr<arrow::StructArray>> ArrowUtils::RemoveFieldFromStructArr
167184
return array;
168185
}
169186

187+
Result<std::shared_ptr<arrow::RecordBatch>> ArrowUtils::NormalizeRecordBatchOffsets(
188+
const std::shared_ptr<arrow::RecordBatch>& record_batch, arrow::MemoryPool* pool) {
189+
arrow::ArrayVector normalized_columns;
190+
for (int32_t i = 0; i < record_batch->num_columns(); ++i) {
191+
const std::shared_ptr<arrow::Array>& column = record_batch->column(i);
192+
if (!HasNonZeroOffset(column->data())) {
193+
continue;
194+
}
195+
if (normalized_columns.empty()) {
196+
normalized_columns = record_batch->columns();
197+
}
198+
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> normalized_column,
199+
arrow::Concatenate({column}, pool));
200+
normalized_columns[i] = std::move(normalized_column);
201+
}
202+
if (normalized_columns.empty()) {
203+
return record_batch;
204+
}
205+
return arrow::RecordBatch::Make(record_batch->schema(), record_batch->num_rows(),
206+
std::move(normalized_columns));
207+
}
208+
170209
Result<arrow::Compression::type> ArrowUtils::GetCompressionType(const std::string& compression) {
171210
std::string normalized = StringUtils::ToLowerCase(compression);
172211
if (normalized.empty() || normalized == "none") {

src/paimon/common/utils/arrow/arrow_utils.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ class PAIMON_EXPORT ArrowUtils {
4848
static Result<std::shared_ptr<arrow::StructArray>> RemoveFieldFromStructArray(
4949
const std::shared_ptr<arrow::StructArray>& struct_array, const std::string& field_name);
5050

51+
static Result<std::shared_ptr<arrow::RecordBatch>> NormalizeRecordBatchOffsets(
52+
const std::shared_ptr<arrow::RecordBatch>& record_batch, arrow::MemoryPool* pool);
53+
5154
static bool EqualsIgnoreNullable(const std::shared_ptr<arrow::DataType>& type,
5255
const std::shared_ptr<arrow::DataType>& other_type);
5356

src/paimon/common/utils/arrow/arrow_utils_test.cpp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,54 @@ TEST(ArrowUtilsTest, TestRemoveFieldFromStructArraySuccess) {
379379
ASSERT_TRUE(result->Equals(expected_struct_array));
380380
}
381381

382+
TEST(ArrowUtilsTest, TestNormalizeRecordBatchOffsets) {
383+
auto value_field = arrow::field("value", arrow::int32());
384+
auto nested_field = arrow::field("nested", arrow::struct_({value_field}));
385+
auto text_field = arrow::field("text", arrow::utf8());
386+
auto clean_field = arrow::field("clean", arrow::boolean());
387+
auto schema = arrow::schema({nested_field, text_field, clean_field});
388+
389+
std::shared_ptr<arrow::Array> values =
390+
arrow::ipc::internal::json::ArrayFromJSON(arrow::int32(), "[0, 1, 2, 3, 4]").ValueOrDie();
391+
std::shared_ptr<arrow::Array> sliced_values = values->Slice(1, 3);
392+
std::shared_ptr<arrow::StructArray> nested_column =
393+
arrow::StructArray::Make({sliced_values}, {value_field->name()}).ValueOrDie();
394+
std::shared_ptr<arrow::Array> text =
395+
arrow::ipc::internal::json::ArrayFromJSON(arrow::utf8(), R"(["a", "b", "c", "d", "e"])")
396+
.ValueOrDie();
397+
std::shared_ptr<arrow::Array> sliced_text = text->Slice(1, 3);
398+
std::shared_ptr<arrow::Array> clean_column =
399+
arrow::ipc::internal::json::ArrayFromJSON(arrow::boolean(), "[true, false, true]")
400+
.ValueOrDie();
401+
std::shared_ptr<arrow::RecordBatch> record_batch = arrow::RecordBatch::Make(
402+
schema, /*num_rows=*/3, {nested_column, sliced_text, clean_column});
403+
404+
ASSERT_EQ(nested_column->offset(), 0);
405+
ASSERT_EQ(nested_column->field(0)->offset(), 1);
406+
ASSERT_EQ(sliced_text->offset(), 1);
407+
ASSERT_EQ(clean_column->offset(), 0);
408+
409+
ASSERT_OK_AND_ASSIGN(
410+
std::shared_ptr<arrow::RecordBatch> normalized_batch,
411+
ArrowUtils::NormalizeRecordBatchOffsets(record_batch, arrow::default_memory_pool()));
412+
ASSERT_NE(normalized_batch.get(), record_batch.get());
413+
ASSERT_TRUE(normalized_batch->Equals(*record_batch));
414+
std::shared_ptr<arrow::StructArray> normalized_nested =
415+
std::static_pointer_cast<arrow::StructArray>(normalized_batch->column(0));
416+
ASSERT_EQ(normalized_nested->offset(), 0);
417+
ASSERT_EQ(normalized_nested->field(0)->offset(), 0);
418+
ASSERT_EQ(normalized_batch->column(1)->offset(), 0);
419+
ASSERT_EQ(normalized_batch->column(2)->offset(), 0);
420+
ASSERT_NE(normalized_batch->column_data(0).get(), record_batch->column_data(0).get());
421+
ASSERT_NE(normalized_batch->column_data(1).get(), record_batch->column_data(1).get());
422+
ASSERT_EQ(normalized_batch->column_data(2).get(), record_batch->column_data(2).get());
423+
424+
ASSERT_OK_AND_ASSIGN(
425+
std::shared_ptr<arrow::RecordBatch> unchanged_batch,
426+
ArrowUtils::NormalizeRecordBatchOffsets(normalized_batch, arrow::default_memory_pool()));
427+
ASSERT_EQ(unchanged_batch.get(), normalized_batch.get());
428+
}
429+
382430
TEST(ArrowUtilsTest, TestEqualsIgnoreNullable) {
383431
{
384432
// test simple
@@ -481,6 +529,7 @@ TEST(ArrowUtilsTest, TestGetCompressionType) {
481529
ASSERT_EQ(type, arrow::Compression::GZIP);
482530
}
483531
{
532+
// test invalid codec
484533
ASSERT_NOK(ArrowUtils::GetCompressionType("invalid_codec"));
485534
}
486535
}

src/paimon/format/parquet/file_reader_wrapper.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "arrow/record_batch.h"
2525
#include "arrow/util/range.h"
2626
#include "fmt/format.h"
27+
#include "paimon/common/utils/arrow/arrow_utils.h"
2728
#include "paimon/format/parquet/column_index_filter.h"
2829
#include "paimon/format/parquet/page_filtered_row_group_reader.h"
2930
#include "paimon/format/parquet/parquet_format_defs.h"
@@ -273,6 +274,10 @@ Result<std::shared_ptr<arrow::RecordBatch>> FileReaderWrapper::NextFullyMatched(
273274
if (!record_batch) {
274275
return std::shared_ptr<arrow::RecordBatch>();
275276
}
277+
// Large binary columns (exceed 2GB) may split at different row boundaries. TableBatchReader
278+
// aligns their chunks by slicing columns, which may leave non-zero child offsets.
279+
PAIMON_ASSIGN_OR_RAISE(record_batch,
280+
ArrowUtils::NormalizeRecordBatchOffsets(record_batch, pool_.get()));
276281

277282
int32_t rg_id = target_row_groups_[current_row_group_idx_].GetRowGroupIndex();
278283
uint64_t rg_end = all_row_group_ranges_[rg_id].second;

src/paimon/format/parquet/page_filtered_row_group_reader.cpp

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "arrow/table.h"
2727
#include "arrow/util/future.h"
2828
#include "fmt/format.h"
29+
#include "paimon/common/utils/arrow/arrow_utils.h"
2930
#include "paimon/common/utils/arrow/status_utils.h"
3031
#include "parquet/arrow/reader_internal.h"
3132
#include "parquet/metadata.h"
@@ -36,12 +37,13 @@ namespace paimon::parquet {
3637
namespace {
3738

3839
/// Wraps an arrow::Table + TableBatchReader as a RecordBatchReader so the caller can
39-
/// stream zero-copy-sliced batches without deep-copying multi-chunk columns. The Table
40-
/// is held to keep its ChunkedArrays alive for the inner TableBatchReader.
40+
/// stream batches while ensuring every returned array offset is zero. The Table is held
41+
/// to keep its ChunkedArrays alive for the inner TableBatchReader.
4142
class TableRecordBatchReader : public arrow::RecordBatchReader {
4243
public:
43-
TableRecordBatchReader(std::shared_ptr<arrow::Table> table, int64_t chunksize)
44-
: table_(std::move(table)), inner_(*table_) {
44+
TableRecordBatchReader(std::shared_ptr<arrow::Table> table, int64_t chunksize,
45+
std::shared_ptr<arrow::MemoryPool> pool)
46+
: table_(std::move(table)), inner_(*table_), pool_(std::move(pool)) {
4547
inner_.set_chunksize(chunksize);
4648
}
4749

@@ -50,12 +52,26 @@ class TableRecordBatchReader : public arrow::RecordBatchReader {
5052
}
5153

5254
arrow::Status ReadNext(std::shared_ptr<arrow::RecordBatch>* out) override {
53-
return inner_.ReadNext(out);
55+
ARROW_RETURN_NOT_OK(inner_.ReadNext(out));
56+
if (!*out) {
57+
return arrow::Status::OK();
58+
}
59+
60+
// Page filtering may produce columns with different chunk boundaries. TableBatchReader
61+
// aligns them by slicing columns, which may leave non-zero child offsets.
62+
Result<std::shared_ptr<arrow::RecordBatch>> normalized_result =
63+
ArrowUtils::NormalizeRecordBatchOffsets(*out, pool_.get());
64+
if (!normalized_result.ok()) {
65+
return ToArrowStatus(normalized_result.status());
66+
}
67+
*out = std::move(normalized_result).value();
68+
return arrow::Status::OK();
5469
}
5570

5671
private:
5772
std::shared_ptr<arrow::Table> table_;
5873
arrow::TableBatchReader inner_;
74+
std::shared_ptr<arrow::MemoryPool> pool_;
5975
};
6076

6177
} // namespace
@@ -240,7 +256,8 @@ Result<std::unique_ptr<arrow::RecordBatchReader>> PageFilteredRowGroupReader::Re
240256
if (row_ranges.IsEmpty()) {
241257
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Table> empty_table,
242258
arrow::Table::MakeEmpty(arrow_schema, pool.get()));
243-
return std::make_unique<TableRecordBatchReader>(std::move(empty_table), max_chunksize);
259+
return std::make_unique<TableRecordBatchReader>(std::move(empty_table), max_chunksize,
260+
pool);
244261
}
245262

246263
int64_t expected_rows = row_ranges.RowCount();
@@ -283,7 +300,8 @@ Result<std::unique_ptr<arrow::RecordBatchReader>> PageFilteredRowGroupReader::Re
283300
}
284301

285302
auto table = arrow::Table::Make(arrow_schema, std::move(columns), expected_rows);
286-
return std::make_unique<TableRecordBatchReader>(std::move(table), max_chunksize);
303+
return std::make_unique<TableRecordBatchReader>(std::move(table), max_chunksize,
304+
std::move(pool));
287305
}
288306

289307
std::vector<::arrow::io::ReadRange> PageFilteredRowGroupReader::ComputePageRanges(

src/paimon/format/parquet/page_filtered_row_group_reader_test.cpp

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -661,14 +661,13 @@ TEST_F(PageFilteredRowGroupReaderTest, ComputePageRangesMultiplePages) {
661661
ASSERT_LT(ranges[0].offset, ranges[1].offset);
662662
}
663663

664-
/// Test: variable-length columns are streamed across multiple zero-copy-sliced
664+
/// Test: variable-length columns are streamed across multiple offset-normalized
665665
/// RecordBatches when batch_size is smaller than the matched row count, instead of
666666
/// being concatenated into a single RecordBatch via CombineChunks.
667667
///
668668
/// This verifies the alignment with Arrow's standard TableBatchReader path:
669-
/// multi-chunk binary/string columns split along chunk + batch_size boundaries,
670-
/// with no deep copy. Asserts both correctness (total rows + full content order) and
671-
/// the multi-batch shape (more than one chunk in the collected ChunkedArray).
669+
/// multi-chunk binary/string columns split along chunk + batch_size boundaries. It
670+
/// asserts correctness and the multi-batch shape.
672671
TEST_F(PageFilteredRowGroupReaderTest, StringColumnMultiBatchStreaming) {
673672
std::string file_name = dir_->Str() + "/string_multi_batch.parquet";
674673

@@ -721,6 +720,23 @@ TEST_F(PageFilteredRowGroupReaderTest, StringColumnMultiBatchStreaming) {
721720
ASSERT_EQ(40, seen);
722721
}
723722

723+
TEST_F(PageFilteredRowGroupReaderTest, NormalizesSlicedBatchOffsets) {
724+
std::string file_name = dir_->Str() + "/normalized_sliced_offsets.parquet";
725+
std::shared_ptr<arrow::StructArray> data = MakeSequentialIntData(60);
726+
WriteTestFile(file_name, data, /*write_batch_size=*/10, /*max_row_group_length=*/60);
727+
728+
std::shared_ptr<arrow::Schema> read_schema =
729+
arrow::schema({arrow::field("val", arrow::int32())});
730+
std::shared_ptr<Predicate> predicate = PredicateBuilder::GreaterOrEqual(
731+
/*field_index=*/0, /*field_name=*/"val", FieldType::INT, Literal(20));
732+
733+
std::shared_ptr<arrow::ChunkedArray> result;
734+
ReadWithPredicateImpl(file_name, read_schema, predicate, &result, /*batch_size=*/7);
735+
ASSERT_TRUE(result);
736+
ASSERT_EQ(40, result->length());
737+
ASSERT_GT(result->num_chunks(), 1);
738+
}
739+
724740
/// Test: end-to-end page-filtered read produces correct results when using page-level PreBuffer.
725741
///
726742
/// This exercises the full path: ComputePageRanges → PreBufferRanges → CachedInputStream →

src/paimon/testing/utils/read_result_collector.h

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ class ReadResultCollector {
180180
if (BatchReader::IsEofBatch(batch_with_bitmap)) {
181181
return std::shared_ptr<arrow::Array>();
182182
}
183+
PAIMON_RETURN_NOT_OK(CheckBatchOffset(batch_with_bitmap.first));
183184
assert(!batch_with_bitmap.second.IsEmpty());
184185
PAIMON_ASSIGN_OR_RAISE(
185186
batch, ReaderUtils::ApplyBitmapToReadBatch(std::move(batch_with_bitmap),
@@ -192,12 +193,30 @@ class ReadResultCollector {
192193
if (BatchReader::IsEofBatch(batch)) {
193194
return std::shared_ptr<arrow::Array>();
194195
}
196+
PAIMON_RETURN_NOT_OK(CheckBatchOffset(batch));
195197
}
196198
auto& [c_array, c_schema] = batch;
197199
assert(c_array->length > 0);
198200
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(auto result_array,
199201
arrow::ImportArray(c_array.get(), c_schema.get()));
200202
return result_array;
201203
}
204+
205+
static Status CheckBatchOffset(const BatchReader::ReadBatch& batch) {
206+
assert(!BatchReader::IsEofBatch(batch));
207+
return CheckArrayOffset(batch.first.get());
208+
}
209+
210+
static Status CheckArrayOffset(const ArrowArray* array) {
211+
assert(array);
212+
if (array->offset != 0) {
213+
return Status::Invalid("BatchReader returned an array with non-zero offset " +
214+
std::to_string(array->offset));
215+
}
216+
for (int64_t i = 0; i < array->n_children; i++) {
217+
PAIMON_RETURN_NOT_OK(CheckArrayOffset(array->children[i]));
218+
}
219+
return Status::OK();
220+
}
202221
};
203222
} // namespace paimon::test

0 commit comments

Comments
 (0)