Skip to content

Commit 4ea9719

Browse files
mrdrivingduckcodex
andcommitted
fix(arrow): retain pools for stream buffers
Keep the Arrow memory pool alive while stream buffers remain referenced. Add deterministic coverage for synchronous and asynchronous reads after adapter teardown. Co-authored-by: GPT-5.6 Terra <codex@users.noreply.github.com>
1 parent 58745c5 commit 4ea9719

2 files changed

Lines changed: 136 additions & 2 deletions

File tree

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,19 @@ arrow::Status ValidateArrowIoRange(int64_t value, const char* name) {
4343
return arrow::Status::OK();
4444
}
4545

46+
struct BufferWithMemoryPool {
47+
std::shared_ptr<arrow::MemoryPool> pool;
48+
std::shared_ptr<arrow::Buffer> buffer;
49+
};
50+
51+
std::shared_ptr<arrow::Buffer> KeepMemoryPoolAlive(std::shared_ptr<arrow::Buffer> buffer,
52+
const std::shared_ptr<arrow::MemoryPool>& pool) {
53+
auto holder =
54+
std::make_shared<BufferWithMemoryPool>(BufferWithMemoryPool{pool, std::move(buffer)});
55+
auto* buffer_ptr = holder->buffer.get();
56+
return std::shared_ptr<arrow::Buffer>(std::move(holder), buffer_ptr);
57+
}
58+
4659
} // namespace
4760

4861
ArrowInputStreamAdapter::ArrowInputStreamAdapter(
@@ -82,7 +95,7 @@ arrow::Result<std::shared_ptr<arrow::Buffer>> ArrowInputStreamAdapter::Read(int6
8295
if (read_bytes < nbytes) {
8396
ARROW_RETURN_NOT_OK(buffer->Resize(read_bytes));
8497
}
85-
return std::shared_ptr<arrow::Buffer>(std::move(buffer));
98+
return KeepMemoryPoolAlive(std::shared_ptr<arrow::Buffer>(std::move(buffer)), pool_);
8699
}
87100

88101
arrow::Result<int64_t> ArrowInputStreamAdapter::ReadAt(int64_t position, int64_t nbytes,
@@ -107,7 +120,7 @@ arrow::Result<std::shared_ptr<arrow::Buffer>> ArrowInputStreamAdapter::ReadAt(in
107120
if (read_bytes < nbytes) {
108121
ARROW_RETURN_NOT_OK(buffer->Resize(read_bytes));
109122
}
110-
return std::shared_ptr<arrow::Buffer>(std::move(buffer));
123+
return KeepMemoryPoolAlive(std::shared_ptr<arrow::Buffer>(std::move(buffer)), pool_);
111124
}
112125

113126
arrow::Future<std::shared_ptr<arrow::Buffer>> ArrowInputStreamAdapter::ReadAsync(
@@ -131,6 +144,7 @@ arrow::Future<std::shared_ptr<arrow::Buffer>> ArrowInputStreamAdapter::ReadAsync
131144
return fut;
132145
}
133146
std::shared_ptr<arrow::Buffer> buffer = std::move(buffer_result).ValueUnsafe();
147+
buffer = KeepMemoryPoolAlive(std::move(buffer), pool_);
134148
std::shared_ptr<std::atomic<uint64_t>> storage_read_bytes = storage_read_bytes_;
135149
input_stream_->ReadAsync(
136150
reinterpret_cast<char*>(buffer->mutable_data()), nbytes, position,

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,11 @@
1818
*/
1919

2020
#include <cstdint>
21+
#include <cstring>
22+
#include <functional>
2123
#include <memory>
2224
#include <string>
25+
#include <utility>
2326

2427
#include "arrow/api.h"
2528
#include "arrow/io/type_fwd.h"
@@ -35,6 +38,79 @@
3538

3639
namespace paimon::test {
3740

41+
namespace {
42+
43+
constexpr char kTestPayload[] = "data";
44+
constexpr int64_t kTestSize = sizeof(kTestPayload) - 1;
45+
46+
class DeferredInputStream : public InputStream {
47+
public:
48+
Status Seek(int64_t, SeekOrigin) override {
49+
return Status::OK();
50+
}
51+
52+
Result<int64_t> GetPos() const override {
53+
return 0;
54+
}
55+
56+
Result<int64_t> Read(char* buffer, int64_t size) override {
57+
if (size != kTestSize) {
58+
return Status::Invalid("unexpected read size");
59+
}
60+
std::memcpy(buffer, kTestPayload, kTestSize);
61+
return kTestSize;
62+
}
63+
64+
Result<int64_t> Read(char* buffer, int64_t size, int64_t) override {
65+
return Read(buffer, size);
66+
}
67+
68+
void ReadAsync(char* buffer, int64_t size, int64_t,
69+
std::function<void(Status)>&& callback) override {
70+
buffer_ = buffer;
71+
size_ = size;
72+
callback_ = std::move(callback);
73+
}
74+
75+
Status Complete() {
76+
if (!callback_) {
77+
return Status::Invalid("async request was not started");
78+
}
79+
if (size_ != kTestSize) {
80+
return Status::Invalid("unexpected async read size");
81+
}
82+
std::memcpy(buffer_, kTestPayload, kTestSize);
83+
auto callback = std::move(callback_);
84+
callback(Status::OK());
85+
return Status::OK();
86+
}
87+
88+
Status Close() override {
89+
return Status::OK();
90+
}
91+
92+
Result<std::string> GetUri() const override {
93+
return std::string("test://input");
94+
}
95+
96+
Result<int64_t> Length() const override {
97+
return kTestSize;
98+
}
99+
100+
private:
101+
char* buffer_ = nullptr;
102+
int64_t size_ = 0;
103+
std::function<void(Status)> callback_;
104+
};
105+
106+
std::shared_ptr<ArrowInputStreamAdapter> CreateAdapter(
107+
const std::shared_ptr<DeferredInputStream>& stream,
108+
const std::shared_ptr<arrow::MemoryPool>& pool) {
109+
return std::make_shared<ArrowInputStreamAdapter>(stream, kTestSize, pool);
110+
}
111+
112+
} // namespace
113+
38114
TEST(ArrowStreamAdapterTest, TestInputAndOutputStream) {
39115
auto test_root_dir = UniqueTestDirectory::Create();
40116
ASSERT_TRUE(test_root_dir);
@@ -91,4 +167,48 @@ TEST(ArrowStreamAdapterTest, TestInputAndOutputStream) {
91167
ASSERT_TRUE(in_stream->closed());
92168
}
93169

170+
TEST(ArrowStreamAdapterTest, TestReadKeepsMemoryPoolAliveUntilBufferReleased) {
171+
auto stream = std::make_shared<DeferredInputStream>();
172+
std::shared_ptr<arrow::MemoryPool> pool(GetArrowPool(GetDefaultPool()));
173+
auto adapter = CreateAdapter(stream, pool);
174+
ASSERT_NE(adapter, nullptr);
175+
176+
std::shared_ptr<arrow::Buffer> buffer = adapter->Read(kTestSize).ValueOrDie();
177+
adapter.reset();
178+
pool.reset();
179+
180+
ASSERT_EQ(buffer->ToString(), kTestPayload);
181+
buffer.reset();
182+
}
183+
184+
TEST(ArrowStreamAdapterTest, TestReadAtKeepsMemoryPoolAliveUntilBufferReleased) {
185+
auto stream = std::make_shared<DeferredInputStream>();
186+
std::shared_ptr<arrow::MemoryPool> pool(GetArrowPool(GetDefaultPool()));
187+
auto adapter = CreateAdapter(stream, pool);
188+
ASSERT_NE(adapter, nullptr);
189+
190+
std::shared_ptr<arrow::Buffer> buffer = adapter->ReadAt(0, kTestSize).ValueOrDie();
191+
adapter.reset();
192+
pool.reset();
193+
194+
ASSERT_EQ(buffer->ToString(), kTestPayload);
195+
buffer.reset();
196+
}
197+
198+
TEST(ArrowStreamAdapterTest, TestAsyncReadKeepsMemoryPoolAliveUntilBufferReleased) {
199+
auto stream = std::make_shared<DeferredInputStream>();
200+
std::shared_ptr<arrow::MemoryPool> pool(GetArrowPool(GetDefaultPool()));
201+
auto adapter = CreateAdapter(stream, pool);
202+
ASSERT_NE(adapter, nullptr);
203+
204+
auto future = adapter->ReadAsync(arrow::io::default_io_context(), 0, kTestSize);
205+
adapter.reset();
206+
pool.reset();
207+
208+
ASSERT_OK(stream->Complete());
209+
std::shared_ptr<arrow::Buffer> buffer = future.MoveResult().ValueOrDie();
210+
ASSERT_EQ(buffer->ToString(), kTestPayload);
211+
buffer.reset();
212+
}
213+
94214
} // namespace paimon::test

0 commit comments

Comments
 (0)