Skip to content

Commit 28dbc7c

Browse files
committed
fix: address avro blob and executor review issues
1 parent 200a487 commit 28dbc7c

12 files changed

Lines changed: 34 additions & 31 deletions

src/paimon/common/data/blob_defs.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ class BlobDefs {
5353
static constexpr int32_t kContentStartOffset = 4;
5454
/// Total metadata length per bin: magic(4) + bin_length(8) + crc32(4) = 16.
5555
static constexpr int32_t kTotalMetaLength = 16;
56-
/// Blob file header length: index_len(4) + version(1) = 5.
57-
static constexpr uint32_t kBlobFileHeaderLength = 5;
56+
/// Blob file footer length: index_len(4) + version(1) = 5.
57+
static constexpr uint32_t kBlobFileFooterLength = 5;
5858
};
5959

6060
} // namespace paimon

src/paimon/common/executor/executor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ void DefaultExecutor::WorkerThread() {
134134
PAIMON_EXPORT std::shared_ptr<Executor> GetGlobalDefaultExecutor() {
135135
static uint32_t all_cores = std::thread::hardware_concurrency();
136136
static std::shared_ptr<Executor> internal =
137-
std::make_shared<DefaultExecutor>(/*thread_count=*/all_cores);
137+
std::make_shared<DefaultExecutor>(/*thread_count=*/all_cores > 0 ? all_cores : 1);
138138
return internal;
139139
}
140140

src/paimon/format/avro/avro_direct_decoder.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ namespace paimon::avro {
3333

3434
class AvroDirectDecoder {
3535
public:
36+
AvroDirectDecoder() = delete;
37+
~AvroDirectDecoder() = delete;
38+
3639
/// Context for reusing scratch buffers during Avro decoding
3740
///
3841
/// Avoids frequent small allocations by reusing temporary buffers across multiple decode

src/paimon/format/avro/avro_direct_encoder.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -354,11 +354,11 @@ Status AvroDirectEncoder::EncodeArrowToAvro(const ::avro::NodePtr& avro_node,
354354
return Status::Invalid(fmt::format("AVRO_MAP keys must be StringArray, got {}",
355355
keys->type()->ToString()));
356356
}
357+
const auto& string_array =
358+
arrow::internal::checked_cast<const arrow::StringArray&>(*keys);
357359

358360
for (int64_t i = start; i < end; ++i) {
359361
encoder->startItem();
360-
const auto& string_array =
361-
arrow::internal::checked_cast<const arrow::StringArray&>(*keys);
362362
std::string_view key_value = string_array.GetView(i);
363363
encoder->encodeString(std::string(key_value));
364364

src/paimon/format/avro/avro_direct_encoder.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ namespace paimon::avro {
3333

3434
class AvroDirectEncoder {
3535
public:
36+
AvroDirectEncoder() = delete;
37+
~AvroDirectEncoder() = delete;
38+
3639
/// Context for reusing scratch buffers during Avro encoding
3740
///
3841
/// Avoids frequent small allocations by reusing temporary buffers across multiple encode

src/paimon/format/avro/avro_file_format.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ AvroFileFormat::AvroFileFormat(const std::map<std::string, std::string>& options
3939

4040
Result<std::unique_ptr<ReaderBuilder>> AvroFileFormat::CreateReaderBuilder(
4141
int32_t batch_size) const {
42-
return std::make_unique<AvroReaderBuilder>(options_, batch_size);
42+
return std::make_unique<AvroReaderBuilder>(batch_size);
4343
}
4444

4545
Result<std::unique_ptr<WriterBuilder>> AvroFileFormat::CreateWriterBuilder(

src/paimon/format/avro/avro_reader_builder.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ namespace paimon::avro {
3232

3333
class AvroReaderBuilder : public ReaderBuilder {
3434
public:
35-
AvroReaderBuilder(const std::map<std::string, std::string>& options, int32_t batch_size)
36-
: batch_size_(batch_size), pool_(GetDefaultPool()), options_(options) {}
35+
explicit AvroReaderBuilder(int32_t batch_size)
36+
: batch_size_(batch_size), pool_(GetDefaultPool()) {}
3737

3838
ReaderBuilder* WithMemoryPool(const std::shared_ptr<MemoryPool>& pool) override {
3939
pool_ = pool;
@@ -52,7 +52,6 @@ class AvroReaderBuilder : public ReaderBuilder {
5252
private:
5353
const int32_t batch_size_;
5454
std::shared_ptr<MemoryPool> pool_;
55-
const std::map<std::string, std::string> options_;
5655
};
5756

5857
} // namespace paimon::avro

src/paimon/format/blob/blob_file_batch_reader.cpp

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
#include "paimon/format/blob/blob_file_batch_reader.h"
1818

1919
#include <algorithm>
20-
#include <future>
2120
#include <numeric>
2221

2322
#include "arrow/api.h"
@@ -27,7 +26,6 @@
2726
#include "arrow/util/bit_util.h"
2827
#include "fmt/format.h"
2928
#include "paimon/common/data/blob_utils.h"
30-
#include "paimon/common/executor/future.h"
3129
#include "paimon/common/io/offset_input_stream.h"
3230
#include "paimon/common/metrics/metrics_impl.h"
3331
#include "paimon/common/utils/arrow/mem_utils.h"
@@ -52,24 +50,24 @@ Result<std::unique_ptr<BlobFileBatchReader>> BlobFileBatchReader::Create(
5250

5351
PAIMON_ASSIGN_OR_RAISE(int64_t file_size, input_stream->Length());
5452
PAIMON_RETURN_NOT_OK(
55-
input_stream->Seek(file_size - BlobDefs::kBlobFileHeaderLength, FS_SEEK_SET));
56-
int8_t header[BlobDefs::kBlobFileHeaderLength];
53+
input_stream->Seek(file_size - BlobDefs::kBlobFileFooterLength, FS_SEEK_SET));
54+
int8_t footer[BlobDefs::kBlobFileFooterLength];
5755
PAIMON_ASSIGN_OR_RAISE(
5856
int64_t actual_size,
59-
input_stream->Read(reinterpret_cast<char*>(header), BlobDefs::kBlobFileHeaderLength));
60-
if (actual_size != BlobDefs::kBlobFileHeaderLength) {
57+
input_stream->Read(reinterpret_cast<char*>(footer), BlobDefs::kBlobFileFooterLength));
58+
if (actual_size != BlobDefs::kBlobFileFooterLength) {
6159
return Status::Invalid(
62-
fmt::format("actual read size {} not match with expect header length {}", actual_size,
63-
BlobDefs::kBlobFileHeaderLength));
60+
fmt::format("actual read size {} not match with expect footer length {}", actual_size,
61+
BlobDefs::kBlobFileFooterLength));
6462
}
65-
int8_t version = header[4];
63+
int8_t version = footer[4];
6664
if (version != BlobDefs::kFileVersion) {
6765
return Status::Invalid(fmt::format(
6866
"create blob format reader failed. unsupported blob file version: {}", version));
6967
}
70-
int32_t index_length = GetIndexLength(header, 0);
68+
int32_t index_length = GetIndexLength(footer, 0);
7169
PAIMON_RETURN_NOT_OK(input_stream->Seek(
72-
file_size - BlobDefs::kBlobFileHeaderLength - index_length, FS_SEEK_SET));
70+
file_size - BlobDefs::kBlobFileFooterLength - index_length, FS_SEEK_SET));
7371
std::vector<char> index_bytes(index_length, '\0');
7472
PAIMON_ASSIGN_OR_RAISE(actual_size, input_stream->Read(index_bytes.data(), index_length));
7573
if (actual_size != index_length) {

src/paimon/format/blob/blob_format_writer.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ BlobFormatWriter::BlobFormatWriter(const std::shared_ptr<OutputStream>& out, con
4545
write_consumer_(std::move(write_consumer)) {
4646
metrics_ = std::make_shared<MetricsImpl>();
4747
tmp_buffer_ = Bytes::AllocateBytes(kTmpBufferSize, pool_.get());
48+
magic_number_bytes_ = IntegerToLittleEndian<int32_t>(BlobDefs::kMagicNumber, pool_);
4849
}
4950

5051
Result<std::unique_ptr<BlobFormatWriter>> BlobFormatWriter::Create(
@@ -159,9 +160,7 @@ Status BlobFormatWriter::WriteBlob(std::string_view blob_data) {
159160
PAIMON_ASSIGN_OR_RAISE(int64_t previous_pos, out_->GetPos());
160161

161162
// write magic number
162-
static PAIMON_UNIQUE_PTR<Bytes> kMagicNumberBytes =
163-
IntegerToLittleEndian<int32_t>(BlobDefs::kMagicNumber, pool_);
164-
PAIMON_RETURN_NOT_OK(WriteWithCrc32(kMagicNumberBytes->data(), kMagicNumberBytes->size()));
163+
PAIMON_RETURN_NOT_OK(WriteWithCrc32(magic_number_bytes_->data(), magic_number_bytes_->size()));
165164

166165
// write blob content
167166
// Dynamically check whether blob_data is a serialized BlobDescriptor (by magic header)
@@ -183,8 +182,9 @@ Status BlobFormatWriter::WriteBlob(std::string_view blob_data) {
183182
while (read_len > 0) {
184183
PAIMON_ASSIGN_OR_RAISE(int64_t actual_read_len, in->Read(tmp_buffer_->data(), read_len));
185184
if (actual_read_len != read_len) {
186-
return Status::Invalid("actual read length {}, not match with expect length {}",
187-
actual_read_len, read_len);
185+
return Status::Invalid(
186+
fmt::format("actual read length {}, not match with expect length {}",
187+
actual_read_len, read_len));
188188
}
189189
PAIMON_RETURN_NOT_OK(WriteWithCrc32(tmp_buffer_->data(), actual_read_len));
190190
total_read_length += actual_read_len;
@@ -212,8 +212,8 @@ Status BlobFormatWriter::WriteBlob(std::string_view blob_data) {
212212
Status BlobFormatWriter::WriteBytes(const char* data, int64_t length) {
213213
PAIMON_ASSIGN_OR_RAISE(int64_t actual, out_->Write(data, length));
214214
if (actual != length) {
215-
return Status::Invalid("not suppose actual length {} not match with expect {}", actual,
216-
length);
215+
return Status::Invalid(
216+
fmt::format("unexpected actual length {} not match with expect {}", actual, length));
217217
}
218218
return Status::OK();
219219
}

src/paimon/format/blob/blob_format_writer.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,15 @@ class BlobFormatWriter : public FormatWriter {
8686
static PAIMON_UNIQUE_PTR<Bytes> IntegerToLittleEndian(T value,
8787
const std::shared_ptr<MemoryPool>& pool);
8888

89-
public:
89+
private:
9090
static constexpr uint32_t kTmpBufferSize = 1024 * 1024;
9191

92-
private:
9392
uint32_t crc32_ = 0;
9493
std::vector<int64_t> bin_lengths_;
9594
std::shared_ptr<OutputStream> out_;
9695
std::string uri_;
9796
PAIMON_UNIQUE_PTR<Bytes> tmp_buffer_;
97+
PAIMON_UNIQUE_PTR<Bytes> magic_number_bytes_;
9898
std::shared_ptr<arrow::DataType> data_type_;
9999
std::shared_ptr<FileSystem> fs_;
100100
std::shared_ptr<MemoryPool> pool_;

0 commit comments

Comments
 (0)