lxy-9602 commented on code in PR #242:
URL: https://github.com/apache/paimon-cpp/pull/242#discussion_r3841628763
##########
src/paimon/format/avro/avro_direct_decoder.h:
##########
@@ -81,6 +81,12 @@ class AvroDirectDecoder {
const std::optional<std::set<size_t>>&
projection,
::avro::Decoder* decoder,
arrow::ArrayBuilder* array_builder,
DecodeContext* ctx);
+
+ /// Reserve slots for a builder and any struct children with the same
cardinality.
+ /// @param array_builder Builder to reserve.
+ /// @param capacity Number of additional values to append.
+ /// @return Status::OK if all reservations succeed.
+ static Status ReserveBuilderCapacity(arrow::ArrayBuilder* array_builder,
int64_t capacity);
};
Review Comment:
Could we move the output parameter to the end?
##########
src/paimon/format/avro/avro_file_batch_reader.cpp:
##########
@@ -123,7 +127,9 @@ Result<BatchReader::ReadBatch>
AvroFileBatchReader::NextBatch() {
}
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> array,
array_builder_->Finish());
+#ifndef NDEBUG
PAIMON_RETURN_NOT_OK_FROM_ARROW(array->Validate());
+#endif
Review Comment:
This looks like the first use of `#ifndef NDEBUG` in the production code.
Could we use `assert` or a similar approach instead?
##########
src/paimon/format/avro/avro_direct_decoder.cpp:
##########
@@ -57,6 +57,20 @@ Status DecodeFieldToBuilder(const ::avro::NodePtr& avro_node,
::avro::Decoder* decoder, arrow::ArrayBuilder*
array_builder,
AvroDirectDecoder::DecodeContext* ctx);
+Status ReserveBuilderCapacityImpl(arrow::ArrayBuilder* array_builder, int64_t
capacity) {
+ PAIMON_RETURN_NOT_OK_FROM_ARROW(array_builder->Reserve(capacity));
+ if (array_builder->type()->id() != arrow::Type::STRUCT) {
+ return Status::OK();
+ }
+
+ auto* struct_builder = checked_cast<arrow::StructBuilder*>(array_builder);
+ for (int32_t i = 0; i < struct_builder->num_fields(); ++i) {
+ PAIMON_RETURN_NOT_OK(
+ ReserveBuilderCapacityImpl(struct_builder->field_builder(i),
capacity));
+ }
+ return Status::OK();
+}
Review Comment:
I didn’t notice an obvious allocation-related hotspot under
ArrayBuilder::Append() in the flame graphs. Have we run any ablation tests to
verify whether pre-reserving capacity actually improves scan latency?
--
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]
