zjw1111 commented on code in PR #260:
URL: https://github.com/apache/paimon-cpp/pull/260#discussion_r3893468302
##########
src/paimon/format/mosaic/mosaic_reader_builder.h:
##########
@@ -30,10 +31,13 @@ namespace paimon::mosaic {
class MosaicReaderBuilder : public ReaderBuilder {
public:
explicit MosaicReaderBuilder(int32_t batch_size)
- : batch_size_(batch_size), pool_(GetDefaultPool()) {}
+ : batch_size_(batch_size),
+ pool_(GetDefaultPool()),
+ arrow_pool_(GetSharedArrowPool(pool_)) {}
ReaderBuilder* WithMemoryPool(const std::shared_ptr<MemoryPool>& pool)
override {
pool_ = pool;
+ arrow_pool_ = GetSharedArrowPool(pool);
Review 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`.
##########
src/paimon/common/reader/reader_utils_test.cpp:
##########
@@ -59,15 +85,32 @@ TEST(ReaderUtilsTest, TestAddAllValidBitmap) {
check_result("[10, 20, 30]");
check_result("");
}
+
+TEST(ReaderUtilsTest, TestCollectResultReleasesBufferedBatchesOnError) {
+ 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);
Review 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.
##########
src/paimon/common/reader/reader_utils.cpp:
##########
@@ -92,12 +94,13 @@ Result<BatchReader::ReadBatch>
ReaderUtils::ApplyBitmapToReadBatch(
PAIMON_ASSIGN_OR_RAISE(arrow::ArrayVector array_vec,
GenerateFilteredArrayVector(arrow_array, bitmap));
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> result,
- arrow::Concatenate(array_vec,
arrow_pool));
+ arrow::Concatenate(array_vec,
arrow_pool.get()));
assert(result && result->length() > 0);
std::unique_ptr<ArrowArray> result_c_array =
std::make_unique<ArrowArray>();
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));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
