Skip to content

Commit f3db380

Browse files
committed
refactor: extract WriteBuffer from MergeTreeWriter
Extract batch buffering logic from MergeTreeWriter into a dedicated WriteBuffer class. WriteBuffer manages batch_vec_, row_kinds_vec_, and current_memory_in_bytes_, and provides Write() and Flush() methods. This is a pure internal refactoring that: - Does not change any external API or behavior - Prepares for future Spill-to-Disk functionality - Improves separation of concerns in MergeTreeWriter
1 parent 84e0e8e commit f3db380

7 files changed

Lines changed: 534 additions & 199 deletions

File tree

src/paimon/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,7 @@ set(PAIMON_CORE_SRCS
239239
core/mergetree/compact/lookup_merge_tree_compact_rewriter.cpp
240240
core/mergetree/compact/changelog_merge_tree_rewriter.cpp
241241
core/mergetree/merge_tree_writer.cpp
242+
core/mergetree/write_buffer.cpp
242243
core/mergetree/levels.cpp
243244
core/mergetree/lookup_levels.cpp
244245
core/migrate/file_meta_utils.cpp
@@ -592,6 +593,7 @@ if(PAIMON_BUILD_TESTS)
592593
core/mergetree/lookup/persist_processor_test.cpp
593594
core/mergetree/drop_delete_reader_test.cpp
594595
core/mergetree/merge_tree_writer_test.cpp
596+
core/mergetree/write_buffer_test.cpp
595597
core/mergetree/sorted_run_test.cpp
596598
core/migrate/file_meta_utils_test.cpp
597599
core/operation/metrics/compaction_metrics_test.cpp

src/paimon/core/mergetree/merge_tree_writer.cpp

Lines changed: 11 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -23,42 +23,30 @@
2323
#include <utility>
2424

2525
#include "arrow/api.h"
26-
#include "arrow/array/array_base.h"
27-
#include "arrow/array/array_binary.h"
28-
#include "arrow/array/array_nested.h"
2926
#include "arrow/c/abi.h"
30-
#include "arrow/c/bridge.h"
3127
#include "arrow/c/helpers.h"
32-
#include "arrow/util/checked_cast.h"
33-
#include "fmt/format.h"
3428
#include "paimon/common/metrics/metrics_impl.h"
3529
#include "paimon/common/table/special_fields.h"
36-
#include "paimon/common/types/data_field.h"
3730
#include "paimon/common/utils/arrow/status_utils.h"
3831
#include "paimon/common/utils/scope_guard.h"
3932
#include "paimon/core/io/async_key_value_producer_and_consumer.h"
4033
#include "paimon/core/io/compact_increment.h"
4134
#include "paimon/core/io/data_file_path_factory.h"
4235
#include "paimon/core/io/data_increment.h"
4336
#include "paimon/core/io/key_value_data_file_writer.h"
44-
#include "paimon/core/io/key_value_in_memory_record_reader.h"
4537
#include "paimon/core/io/key_value_meta_projection_consumer.h"
4638
#include "paimon/core/io/key_value_record_reader.h"
4739
#include "paimon/core/io/row_to_arrow_array_converter.h"
4840
#include "paimon/core/io/single_file_writer.h"
4941
#include "paimon/core/manifest/file_source.h"
5042
#include "paimon/core/mergetree/compact/sort_merge_reader_with_loser_tree.h"
43+
#include "paimon/core/mergetree/write_buffer.h"
5144
#include "paimon/core/utils/commit_increment.h"
52-
#include "paimon/data/decimal.h"
5345
#include "paimon/format/file_format.h"
5446
#include "paimon/format/writer_builder.h"
5547
#include "paimon/metrics.h"
5648

5749
namespace paimon {
58-
class FieldsComparator;
59-
class MemoryPool;
60-
template <typename T>
61-
class MergeFunctionWrapper;
6250
class FormatStatsExtractor;
6351

6452
MergeTreeWriter::MergeTreeWriter(
@@ -71,7 +59,6 @@ MergeTreeWriter::MergeTreeWriter(
7159
const CoreOptions& options, const std::shared_ptr<CompactManager>& compact_manager,
7260
const std::shared_ptr<MemoryPool>& pool)
7361
: last_sequence_number_(last_sequence_number + 1),
74-
current_memory_in_bytes_(0),
7562
pool_(pool),
7663
trimmed_primary_keys_(trimmed_primary_keys),
7764
options_(options),
@@ -80,10 +67,12 @@ MergeTreeWriter::MergeTreeWriter(
8067
user_defined_seq_comparator_(user_defined_seq_comparator),
8168
merge_function_wrapper_(merge_function_wrapper),
8269
schema_id_(schema_id),
83-
value_type_(arrow::struct_(value_schema->fields())),
8470
compact_manager_(compact_manager),
8571
metrics_(std::make_shared<MetricsImpl>()) {
8672
write_schema_ = SpecialFields::CompleteSequenceAndValueKindField(value_schema);
73+
write_buffer_ = std::make_unique<WriteBuffer>(
74+
arrow::struct_(value_schema->fields()), trimmed_primary_keys_,
75+
options_.GetSequenceField(), key_comparator_, merge_function_wrapper_, pool_);
8776
}
8877

8978
Status MergeTreeWriter::DoClose() {
@@ -114,8 +103,7 @@ Status MergeTreeWriter::DoClose() {
114103
[[maybe_unused]] auto s = options_.GetFileSystem()->Delete(path_factory_->ToPath(file));
115104
}
116105

117-
batch_vec_.clear();
118-
row_kinds_vec_.clear();
106+
write_buffer_->Clear();
119107
new_files_.clear();
120108
deleted_files_.clear();
121109
compact_before_.clear();
@@ -129,23 +117,8 @@ Status MergeTreeWriter::DoClose() {
129117
}
130118

131119
Status MergeTreeWriter::Write(std::unique_ptr<RecordBatch>&& moved_batch) {
132-
if (ArrowArrayIsReleased(moved_batch->GetData())) {
133-
return Status::Invalid("invalid batch: data is released");
134-
}
135-
std::unique_ptr<RecordBatch> batch = std::move(moved_batch);
136-
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> arrow_array,
137-
arrow::ImportArray(batch->GetData(), value_type_));
138-
auto value_struct_array =
139-
arrow::internal::checked_pointer_cast<arrow::StructArray>(arrow_array);
140-
if (value_struct_array == nullptr) {
141-
return Status::Invalid("invalid RecordBatch: cannot cast to StructArray");
142-
}
143-
PAIMON_ASSIGN_OR_RAISE(int64_t memory_in_bytes, EstimateMemoryUse(value_struct_array));
144-
current_memory_in_bytes_ += memory_in_bytes;
145-
146-
batch_vec_.push_back(std::move(value_struct_array));
147-
row_kinds_vec_.push_back(batch->GetRowKind());
148-
if (current_memory_in_bytes_ >= options_.GetWriteBufferSize()) {
120+
PAIMON_RETURN_NOT_OK(write_buffer_->Write(std::move(moved_batch)));
121+
if (write_buffer_->GetMemoryUsage() >= options_.GetWriteBufferSize()) {
149122
return Flush(/*wait_for_latest_compaction=*/false, /*forced_full_compaction=*/false);
150123
}
151124
return Status::OK();
@@ -247,25 +220,13 @@ Result<bool> MergeTreeWriter::CompactNotCompleted() {
247220
}
248221

249222
Status MergeTreeWriter::Flush(bool wait_for_latest_compaction, bool forced_full_compaction) {
250-
if (!batch_vec_.empty()) {
223+
if (!write_buffer_->IsEmpty()) {
251224
if (compact_manager_->ShouldWaitForLatestCompaction()) {
252225
wait_for_latest_compaction = true;
253226
}
254-
// 1. create key value iter for each record batch
255-
std::vector<std::unique_ptr<KeyValueRecordReader>> readers;
256-
readers.reserve(batch_vec_.size());
257-
for (size_t i = 0; i < batch_vec_.size(); ++i) {
258-
int64_t sequence_number = last_sequence_number_;
259-
last_sequence_number_ += batch_vec_[i]->length();
260-
auto in_memory_reader = std::make_unique<KeyValueInMemoryRecordReader>(
261-
sequence_number, std::move(batch_vec_[i]), std::move(row_kinds_vec_[i]),
262-
trimmed_primary_keys_, options_.GetSequenceField(), key_comparator_,
263-
merge_function_wrapper_, pool_);
264-
readers.push_back(std::move(in_memory_reader));
265-
}
266-
batch_vec_.clear();
267-
row_kinds_vec_.clear();
268-
current_memory_in_bytes_ = 0;
227+
// 1. flush write buffer to get in-memory readers
228+
PAIMON_ASSIGN_OR_RAISE(std::vector<std::unique_ptr<KeyValueRecordReader>> readers,
229+
write_buffer_->Flush(last_sequence_number_));
269230
// 2. prepare loser tree sort merge reader
270231
auto sort_merge_reader = std::make_unique<SortMergeReaderWithLoserTree>(
271232
std::move(readers), key_comparator_, user_defined_seq_comparator_,
@@ -354,68 +315,4 @@ MergeTreeWriter::CreateRollingRowWriter() const {
354315
options_.GetTargetFileSize(/*has_primary_key=*/true), create_file_writer);
355316
}
356317

357-
Result<int64_t> MergeTreeWriter::EstimateMemoryUse(const std::shared_ptr<arrow::Array>& array) {
358-
arrow::Type::type type = array->type()->id();
359-
int64_t null_bits_size_in_bytes = (array->length() + 7) / 8;
360-
switch (type) {
361-
case arrow::Type::type::BOOL:
362-
return null_bits_size_in_bytes + array->length() * sizeof(bool);
363-
case arrow::Type::type::INT8:
364-
return null_bits_size_in_bytes + array->length() * sizeof(int8_t);
365-
case arrow::Type::type::INT16:
366-
return null_bits_size_in_bytes + array->length() * sizeof(int16_t);
367-
case arrow::Type::type::INT32:
368-
return null_bits_size_in_bytes + array->length() * sizeof(int32_t);
369-
case arrow::Type::type::DATE32:
370-
return null_bits_size_in_bytes + array->length() * sizeof(int32_t);
371-
case arrow::Type::type::INT64:
372-
return null_bits_size_in_bytes + array->length() * sizeof(int64_t);
373-
case arrow::Type::type::FLOAT:
374-
return null_bits_size_in_bytes + array->length() * sizeof(float);
375-
case arrow::Type::type::DOUBLE:
376-
return null_bits_size_in_bytes + array->length() * sizeof(double);
377-
case arrow::Type::type::TIMESTAMP:
378-
return null_bits_size_in_bytes + array->length() * sizeof(int64_t);
379-
case arrow::Type::type::DECIMAL:
380-
return null_bits_size_in_bytes + array->length() * sizeof(Decimal::int128_t);
381-
case arrow::Type::type::STRING:
382-
case arrow::Type::type::BINARY: {
383-
auto binary_array =
384-
arrow::internal::checked_cast<const arrow::BinaryArray*>(array.get());
385-
assert(binary_array);
386-
int64_t value_length = binary_array->total_values_length();
387-
int64_t offset_length = array->length() * sizeof(int32_t);
388-
return null_bits_size_in_bytes + value_length + offset_length;
389-
}
390-
case arrow::Type::type::LIST: {
391-
auto list_array = arrow::internal::checked_cast<const arrow::ListArray*>(array.get());
392-
assert(list_array);
393-
PAIMON_ASSIGN_OR_RAISE(int64_t value_mem, EstimateMemoryUse(list_array->values()));
394-
return null_bits_size_in_bytes + value_mem;
395-
}
396-
case arrow::Type::type::MAP: {
397-
auto map_array = arrow::internal::checked_cast<const arrow::MapArray*>(array.get());
398-
assert(map_array);
399-
PAIMON_ASSIGN_OR_RAISE(int64_t key_mem, EstimateMemoryUse(map_array->keys()));
400-
PAIMON_ASSIGN_OR_RAISE(int64_t item_mem, EstimateMemoryUse(map_array->items()));
401-
return null_bits_size_in_bytes + key_mem + item_mem;
402-
}
403-
case arrow::Type::type::STRUCT: {
404-
auto struct_array =
405-
arrow::internal::checked_cast<const arrow::StructArray*>(array.get());
406-
assert(struct_array);
407-
int64_t struct_mem = 0;
408-
for (const auto& field : struct_array->fields()) {
409-
PAIMON_ASSIGN_OR_RAISE(int64_t field_mem, EstimateMemoryUse(field));
410-
struct_mem += field_mem;
411-
}
412-
return null_bits_size_in_bytes + struct_mem;
413-
}
414-
default:
415-
assert(false);
416-
return Status::Invalid(fmt::format("Do not support type {} in EstimateMemoryUse",
417-
array->type()->ToString()));
418-
}
419-
}
420-
421318
} // namespace paimon

src/paimon/core/mergetree/merge_tree_writer.h

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "paimon/core/io/rolling_file_writer.h"
2929
#include "paimon/core/key_value.h"
3030
#include "paimon/core/mergetree/compact/merge_function_wrapper.h"
31+
#include "paimon/core/mergetree/write_buffer.h"
3132
#include "paimon/core/utils/batch_writer.h"
3233
#include "paimon/core/utils/commit_increment.h"
3334
#include "paimon/core/utils/fields_comparator.h"
@@ -95,11 +96,8 @@ class MergeTreeWriter : public BatchWriter {
9596
Status UpdateCompactResult(const std::shared_ptr<CompactResult>& compact_result);
9697
Status UpdateCompactDeletionFile(const std::shared_ptr<CompactDeletionFile>& new_deletion_file);
9798

98-
static Result<int64_t> EstimateMemoryUse(const std::shared_ptr<arrow::Array>& array);
99-
10099
private:
101100
int64_t last_sequence_number_;
102-
int64_t current_memory_in_bytes_;
103101
std::shared_ptr<MemoryPool> pool_;
104102
std::vector<std::string> trimmed_primary_keys_;
105103
CoreOptions options_;
@@ -109,13 +107,11 @@ class MergeTreeWriter : public BatchWriter {
109107
std::shared_ptr<MergeFunctionWrapper<KeyValue>> merge_function_wrapper_;
110108
int64_t schema_id_;
111109
// write_schema = value_schema + special fields
112-
std::shared_ptr<arrow::DataType> value_type_;
113110
std::shared_ptr<arrow::Schema> write_schema_;
114111

115112
std::shared_ptr<CompactManager> compact_manager_;
116113

117-
std::vector<std::shared_ptr<arrow::StructArray>> batch_vec_;
118-
std::vector<std::vector<RecordBatch::RowKind>> row_kinds_vec_;
114+
std::unique_ptr<WriteBuffer> write_buffer_;
119115

120116
std::shared_ptr<Metrics> metrics_;
121117

src/paimon/core/mergetree/merge_tree_writer_test.cpp

Lines changed: 12 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,14 @@
2121
#include <map>
2222
#include <optional>
2323
#include <utility>
24-
#include <variant>
2524

2625
#include "arrow/api.h"
2726
#include "arrow/array/array_base.h"
28-
#include "arrow/array/array_nested.h"
2927
#include "arrow/c/abi.h"
3028
#include "arrow/c/bridge.h"
3129
#include "arrow/ipc/json_simple.h"
3230
#include "gtest/gtest.h"
3331
#include "paimon/common/factories/io_hook.h"
34-
#include "paimon/common/fs/external_path_provider.h"
3532
#include "paimon/common/table/special_fields.h"
3633
#include "paimon/common/types/data_field.h"
3734
#include "paimon/common/utils/scope_guard.h"
@@ -50,7 +47,6 @@
5047
#include "paimon/fs/file_system.h"
5148
#include "paimon/fs/local/local_file_system.h"
5249
#include "paimon/memory/memory_pool.h"
53-
#include "paimon/metrics.h"
5450
#include "paimon/testing/utils/binary_row_generator.h"
5551
#include "paimon/testing/utils/io_exception_helper.h"
5652
#include "paimon/testing/utils/read_result_collector.h"
@@ -88,14 +84,21 @@ class MergeTreeWriterTest : public ::testing::Test {
8884
}
8985
void TearDown() override {}
9086

91-
void WriteBatch(const std::shared_ptr<arrow::Array>& array,
92-
const std::vector<RecordBatch::RowKind>& row_kinds,
93-
MergeTreeWriter* writer) const {
87+
std::unique_ptr<RecordBatch> CreateBatch(
88+
const std::shared_ptr<arrow::Array>& array,
89+
const std::vector<RecordBatch::RowKind>& row_kinds) const {
9490
::ArrowArray c_array;
95-
ASSERT_TRUE(arrow::ExportArray(*array, &c_array).ok());
91+
EXPECT_TRUE(arrow::ExportArray(*array, &c_array).ok());
9692
RecordBatchBuilder batch_builder(&c_array);
9793
batch_builder.SetRowKinds(row_kinds);
98-
ASSERT_OK_AND_ASSIGN(std::unique_ptr<RecordBatch> batch, batch_builder.Finish());
94+
EXPECT_OK_AND_ASSIGN(std::unique_ptr<RecordBatch> batch, batch_builder.Finish());
95+
return batch;
96+
}
97+
98+
void WriteBatch(const std::shared_ptr<arrow::Array>& array,
99+
const std::vector<RecordBatch::RowKind>& row_kinds,
100+
MergeTreeWriter* writer) const {
101+
auto batch = CreateBatch(array, row_kinds);
99102
ASSERT_OK(writer->Write(std::move(batch)));
100103
}
101104

@@ -761,76 +764,6 @@ TEST_F(MergeTreeWriterTest, TestIOException) {
761764
ASSERT_TRUE(run_complete);
762765
}
763766

764-
TEST_F(MergeTreeWriterTest, TestEstimateMemoryUse) {
765-
{
766-
// test simple
767-
std::shared_ptr<arrow::Array> array =
768-
arrow::ipc::internal::json::ArrayFromJSON(value_type_, R"([
769-
["Lucy", 20, 1, 14.1],
770-
["Paul", 20, 1, null],
771-
["Alice", 10, 0, 13.1]
772-
])")
773-
.ValueOrDie();
774-
ASSERT_OK_AND_ASSIGN(int64_t memory_use, MergeTreeWriter::EstimateMemoryUse(array));
775-
int64_t expected_memory_use =
776-
1 + (13 + 3 * 4 + 1) + (3 * 4 + 1) + (3 * 4 + 1) + (3 * 8 + 1);
777-
ASSERT_EQ(memory_use, expected_memory_use);
778-
}
779-
{
780-
// test primitive type
781-
arrow::FieldVector fields = {arrow::field("v0", arrow::boolean()),
782-
arrow::field("v1", arrow::int8()),
783-
arrow::field("v2", arrow::int16()),
784-
arrow::field("v3", arrow::int32()),
785-
arrow::field("v4", arrow::int64()),
786-
arrow::field("v5", arrow::float32()),
787-
arrow::field("v6", arrow::float64()),
788-
arrow::field("v7", arrow::date32()),
789-
arrow::field("v8", arrow::timestamp(arrow::TimeUnit::NANO)),
790-
arrow::field("v9", arrow::decimal128(30, 20)),
791-
arrow::field("v10", arrow::utf8()),
792-
arrow::field("v11", arrow::binary())};
793-
794-
auto array = std::dynamic_pointer_cast<arrow::StructArray>(
795-
arrow::ipc::internal::json::ArrayFromJSON(arrow::struct_(fields), R"([
796-
[true, 10, 200, 65536, 123456789, 0.0, 0.0, 2000, -86399999999500, "2134.48690000000000000009", "difference", "Alice"],
797-
[false, -128, -32768, -2147483648, -9223372036854775808, -3.4028235E38, -1.7976931348623157E308, -719528, -9223372036854775808, "-999999999999999999.99999999999999999999", "Alice", "Two"],
798-
[true, 127, 32767, 2147483647, 9223372036854775807, 3.4028235E38, 1.7976931348623157E308, 2932896, 9223372036854775807, "999999999999999999.99999999999999999999", "Alice", "made"],
799-
[true, 0, 0, 0, 0, 1.4E-45, 4.9E-324, 0, 0, "0.00000000000000000000", "Alice", "wood"]
800-
])")
801-
.ValueOrDie());
802-
ASSERT_OK_AND_ASSIGN(int64_t memory_use, MergeTreeWriter::EstimateMemoryUse(array));
803-
int64_t expected_memory_use = 1 + (4 + 1) + (4 + 1) + (2 * 4 + 1) + (4 * 4 + 1) +
804-
(8 * 4 + 1) + (4 * 4 + 1) + (8 * 4 + 1) + (4 * 4 + 1) +
805-
(8 * 4 + 1) + (4 * 16 + 1) + (25 + 4 * 4 + 1) +
806-
(16 + 4 * 4 + 1);
807-
ASSERT_EQ(memory_use, expected_memory_use);
808-
}
809-
{
810-
// test nested type
811-
arrow::FieldVector fields = {
812-
arrow::field("f0", arrow::list(arrow::int32())),
813-
arrow::field("f1", arrow::map(arrow::utf8(), arrow::int64())),
814-
arrow::field("f2", arrow::struct_({arrow::field("sub1", arrow::int64()),
815-
arrow::field("sub2", arrow::float64()),
816-
arrow::field("sub3", arrow::boolean())})),
817-
};
818-
auto array = std::dynamic_pointer_cast<arrow::StructArray>(
819-
arrow::ipc::internal::json::ArrayFromJSON(arrow::struct_({fields}), R"([
820-
[[1, 2, 3], [["apple", 3], ["banana", 4]], [10, 10.1, false]],
821-
[[4, 5], [["cat", 5], ["dog", 6], ["mouse", 7]], [20, 20.1, true]],
822-
[[6], [["elephant", 7], ["fox", 8]], [null, 30.1, true]]
823-
])")
824-
.ValueOrDie());
825-
ASSERT_OK_AND_ASSIGN(int64_t memory_use, MergeTreeWriter::EstimateMemoryUse(array));
826-
int64_t list_mem = 1 + (4 * 6 + 1);
827-
int64_t map_mem = 1 + (33 + 4 * 7 + 1) + (8 * 7 + 1);
828-
int64_t struct_mem = 1 + (8 * 3 + 1) + (8 * 3 + 1) + (1 * 3 + 1);
829-
int64_t expected_memory_use = 1 + list_mem + map_mem + struct_mem;
830-
ASSERT_EQ(memory_use, expected_memory_use);
831-
}
832-
}
833-
834767
TEST_F(MergeTreeWriterTest, TestBulkData) {
835768
// each batch is a file due to WRITE_BUFFER_SIZE
836769
ASSERT_OK_AND_ASSIGN(

0 commit comments

Comments
 (0)