feat(read): decouple read batch lifetime from readers - #260
Conversation
682e58c to
e167dbf
Compare
| /// retried, as it will repeatedly return the same error code. | ||
| /// \note IMPORTANT: A non-EOF ArrowArray and all its nested child arrays must have offset 0 to | ||
| /// avoid potential issues during conversion through the Arrow C Data Interface. | ||
| /// \note A returned ArrowArray must retain every allocator and plugin resource needed by its |
There was a problem hiding this comment.
use warning or important, and change \ to @
|
|
||
| Status RetainArrowArrayMemoryPool(ArrowArray* array, | ||
| const std::shared_ptr<arrow::MemoryPool>& arrow_pool); | ||
| PAIMON_EXPORT std::shared_ptr<arrow::MemoryPool> GetSharedArrowPool( |
There was a problem hiding this comment.
delete GetSharedArrowPool, GetArrowPool -> shared_ptr
zjw1111
left a comment
zjw1111
left a comment
There was a problem hiding this comment.
Three issues found in the current revision.
|
|
||
| ReaderBuilder* WithMemoryPool(const std::shared_ptr<MemoryPool>& pool) override { | ||
| pool_ = pool; | ||
| arrow_pool_ = GetSharedArrowPool(pool); |
There was a problem hiding this comment.
WithMemoryPool(nullptr) now calls GetSharedArrowPool(nullptr), whose adaptor dereferences the shared pointer in its initializer. This crashes before Build() can return its existing Invalid status. The same eager-construction path exists in OrcReaderBuilder through OrcReadMemory.
| std::unique_ptr<ArrowSchema> result_c_schema = std::make_unique<ArrowSchema>(); | ||
| PAIMON_RETURN_NOT_OK_FROM_ARROW( | ||
| arrow::ExportArray(*result, result_c_array.get(), result_c_schema.get())); | ||
| PAIMON_RETURN_NOT_OK(AddArrowArrayLifetime(result_c_array.get(), arrow_pool)); |
There was a problem hiding this comment.
When AddArrowArrayLifetime fails because the lifetime is empty or its allocation throws std::bad_alloc, it releases only result_c_array. This return path then destroys result_c_schema as a plain unique_ptr without invoking ArrowSchemaRelease, leaking the schema private data exported above. The same pattern appears at the other ExportArray plus AddArrowArrayLifetime call sites.
| auto array = arrow::ipc::internal::json::ArrayFromJSON(arrow::int32(), "[1]").ValueOrDie(); | ||
| ASSERT_OK_AND_ASSIGN(BatchReader::ReadBatch batch, ReadResultCollector::GetReadBatch(array)); | ||
|
|
||
| std::shared_ptr<int> lifetime = std::make_shared<int>(1); |
There was a problem hiding this comment.
This new test uses plain int for the lifetime state. The project code-style rule requires fixed-width integer types and explicitly disallows plain int, including in tests.
2 participants
Purpose
Linked issue: close #259
Arrow and ORC buffers may be allocated from memory-pool adaptors owned by BatchReader. Because these buffers still access their allocator when they are released, Paimon previously required the BatchReader to outlive every ReadBatch returned by it.
This lifetime constraint couples readers with their returned batches and makes integration with downstream engines inconvenient and error-prone, as described in alibaba/paimon-cpp#126.
Inspired by #224 this PR removes that lifetime constraint for batches represented through the Arrow C Data Interface.
The memory-pool resources used by a batch, including Arrow and ORC pools, are retained by the root ArrowArray. They remain alive until the corresponding ArrowArray::release callback finishes. Therefore, a returned batch can safely outlive its BatchReader.
Consumers must treat the root ArrowArray as a complete ownership unit: keep, move, and release the entire structure through the Arrow C Data Interface. Moving individual child arrays out of the root ArrowArray is unsupported because those children do not independently retain the batch’s memory-pool lifetime.
Tests
API and Format
API impact:
TableRead::CreateReader(const std::vector<std::shared_ptr<Split>>&)is nowpure virtual, so concrete TableRead implementations must implement it.
Documentation
Generative AI tooling
Generated-by: Codex (GPT-5)