feat: eliminate GenericDatum in Avro reader for performance #374

shangxinli · 2025-11-30T19:34:14Z

Replace GenericDatum intermediate layer with direct Avro decoder access to improve manifest I/O performance.

Changes:

Add avro_direct_decoder_internal.h with DecodeAvroToBuilder API
Add avro_direct_decoder.cc implementing direct Avro→Arrow decoding
- Primitive types: bool, int, long, float, double, string, binary, fixed
- Temporal types: date, time, timestamp
- Logical types: uuid, decimal
- Nested types: struct, list, map
- Union type handling for nullable fields
Modify avro_reader.cc to use DataFileReaderBase with direct decoder
- Replace DataFileReader with DataFileReaderBase
- Use decoder.decodeInt(), decodeLong(), etc. directly
- Remove GenericDatum allocation and extraction overhead
Update CMakeLists.txt to include new decoder source

Performance improvement:

Before: Avro binary → GenericDatum → Extract → Arrow (3 steps)
After: Avro binary → decoder.decodeInt() → Arrow (2 steps)

This matches Java implementation which uses Decoder directly via ValueReader interface, avoiding intermediate object allocation.

All avro_test cases pass.

Issue: #332

Replace GenericDatum intermediate layer with direct Avro decoder access to improve manifest I/O performance. Changes: - Add avro_direct_decoder_internal.h with DecodeAvroToBuilder API - Add avro_direct_decoder.cc implementing direct Avro→Arrow decoding - Primitive types: bool, int, long, float, double, string, binary, fixed - Temporal types: date, time, timestamp - Logical types: uuid, decimal (with validation) - Nested types: struct, list, map - Union type handling with bounds checking - Field skipping with proper multi-block handling for arrays/maps - Modify avro_reader.cc to use DataFileReaderBase with direct decoder - Replace DataFileReader<GenericDatum> with DataFileReaderBase - Use decoder.decodeInt(), decodeLong(), etc. directly - Remove GenericDatum allocation and extraction overhead - Update CMakeLists.txt to include new decoder source Validation added: - Union branch bounds checking - Decimal byte width validation (uses schema fixedSize, not calculated) - Decimal precision sufficiency validation - Logical type presence validation - Type mismatch error handling Documentation: - Comprehensive API documentation in header - Schema evolution handling via SchemaProjection explained - Error handling behavior documented - Limitations noted (default values not supported) Performance improvement: - Before: Avro binary → GenericDatum → Extract → Arrow (3 steps) - After: Avro binary → decoder.decodeInt() → Arrow (2 steps) This matches Java implementation which uses Decoder directly via ValueReader interface, avoiding intermediate object allocation. All 173 avro_test cases pass. Issue: apache#332

wgtmac · 2025-12-01T01:48:25Z

src/iceberg/avro/avro_reader.cc

      ICEBERG_RETURN_UNEXPECTED(
-          AppendDatumToBuilder(reader_->readerSchema().root(), *context_->datum_,
-                               projection_, *read_schema_, context_->builder_.get()));
+          DecodeAvroToBuilder(reader_->readerSchema().root(), reader_->decoder(),


I think it would be better to use a feature flag to enable users to use the old reader just in case there is any bug.

Good suggestion!

wgtmac · 2025-12-02T16:01:24Z

Could you help add some test cases? The original avro reader is simply a wrapper on top of avro-cpp so existing test cases are very minimal. We need better test coverage to be confident. Perhaps we can use the avro writer to create in-memory avro files (using MakeMockFileIO and then use the new reader to read them. It is also nice to add some benchmarks to prove that this indeed improves the reading performance compared to the original implementation.

Add extensive test coverage to validate the direct decoder implementation: - All primitive types (boolean, int, long, float, double, string, binary) - Temporal types (date, time, timestamp) - Complex nested structures (nested structs, lists, maps) - Null handling and optional fields - Large datasets (1000+ rows) - Direct decoder vs GenericDatum comparison tests Add benchmark tool to measure performance improvements: - Benchmarks with various data patterns (primitives, nested, lists, nulls) - Compares direct decoder vs GenericDatum performance - Expected speedup: 1.5x - 2.5x due to eliminated intermediate copies Add feature flag for direct Avro decoder: - ReaderProperties::kAvroUseDirectDecoder (default: true) - Allows fallback to GenericDatum implementation if issues arise - Dual-path implementation with helper functions to reduce code duplication Test results: - 16 comprehensive Avro reader tests (vs 5 before) - 180 total tests in avro_test suite - 100% passing rate This addresses review feedback from wgtmac to provide better test coverage and prove performance improvements of the direct decoder implementation.

Smith-Cruise · 2025-12-09T14:32:33Z

src/iceberg/avro/avro_direct_decoder.cc

+Status SkipAvroValue(const ::avro::NodePtr& avro_node, ::avro::Decoder& decoder) {
+  switch (avro_node->type()) {
+    case ::avro::AVRO_NULL:
+      // Nothing to skip


Better to call decoder.decodeNull()? Even if nothing todo.

Address reviewer comment to explicitly call decoder.decodeNull() even though AVRO_NULL has no data to skip. This is more consistent with other type handlers and makes the decoder state handling explicit.

wgtmac · 2025-12-10T15:13:26Z

src/iceberg/avro/avro_reader.cc

  SchemaProjection projection_;
-  // The avro reader to read the data into a datum.
-  std::unique_ptr<::avro::DataFileReader<::avro::GenericDatum>> reader_;
+  // The avro reader base - provides direct access to decoder (new path).


I would rather not call them new or legacy path...

wgtmac · 2025-12-11T06:43:55Z

src/iceberg/avro/avro_direct_decoder.cc

+                               ToString(avro_node));
+      }
+      auto* builder = internal::checked_cast<::arrow::BinaryBuilder*>(array_builder);
+      std::vector<uint8_t> bytes;


Do we want to avoid frequent small object allocation like this? Perhaps we can reuse it by adding a class DecodeContext where a std::vector<uint8_t> scratch object is supposed to be used from it.

Same for other temp vector and string variables.

wgtmac · 2025-12-11T07:50:02Z

src/iceberg/avro/avro_direct_decoder.cc

+        return InvalidArgument("Expected Avro fixed for decimal field, got: {}",
+                               ToString(avro_node));
+      }
+      if (avro_node->logicalType().type() != ::avro::LogicalType::DECIMAL) {


Can we combine the two ifs above just like other branches?

wgtmac · 2025-12-11T07:55:33Z

src/iceberg/avro/avro_direct_decoder.cc

+
+      // Validate byte width is sufficient for precision
+      // Max value with P digits: 10^P - 1, needs ceil(P * log(10) / log(256)) bytes
+      size_t min_bytes = (decimal_type.precision() * 415) / 1000 + 1;  // ceil(P * 0.415)
+      if (byte_width < min_bytes) {
+        return InvalidArgument(
+            "Decimal byte width {} insufficient for precision {}, need at least {} bytes",
+            byte_width, decimal_type.precision(), min_bytes);
+      }
+


Suggested change

// Validate byte width is sufficient for precision

// Max value with P digits: 10^P - 1, needs ceil(P * log(10) / log(256)) bytes

size_t min_bytes = (decimal_type.precision() * 415) / 1000 + 1; // ceil(P * 0.415)

if (byte_width < min_bytes) {

return InvalidArgument(

"Decimal byte width {} insufficient for precision {}, need at least {} bytes",

byte_width, decimal_type.precision(), min_bytes);

}

I don't think we need this check.

wgtmac · 2025-12-11T08:12:28Z

src/iceberg/avro/avro_direct_decoder.cc

+  // Handle null fields (fields in projection but not in Avro)
+  for (size_t proj_idx = 0; proj_idx < projections.size(); ++proj_idx) {
+    const auto& field_projection = projections[proj_idx];
+    if (field_projection.kind == FieldProjection::Kind::kNull) {


Return error for other unsupported kind?

wgtmac · 2025-12-11T08:44:18Z

src/iceberg/file_reader.h


+  /// \brief Use direct Avro decoder (true) or GenericDatum-based decoder (false).
+  /// Default: true (use direct decoder for better performance).
+  inline static Entry<bool> kAvroUseDirectDecoder{"avro.use-direct-decoder", true};


I would call it read.avro.skip-datum, WDYT?

wgtmac · 2025-12-11T08:45:21Z

src/iceberg/test/avro_test.cc

+  WriteAndVerify(schema, expected_string);
+}
+
+// Skip Fixed and UUID tests for now - they require specific binary encoding


Suggested change

// Skip Fixed and UUID tests for now - they require specific binary encoding

wgtmac · 2025-12-11T08:47:14Z

src/iceberg/test/avro_test.cc

  WriteAndVerify(schema, expected_string);
 }

+// Comprehensive tests using in-memory MockFileIO


Suggested change

// Comprehensive tests using in-memory MockFileIO

wgtmac · 2025-12-11T08:51:42Z

src/iceberg/test/avro_test.cc

+  WriteAndVerify(schema, expected_string);
+}
+
+TEST_F(AvroReaderTest, MapType) {


Can we test a map with a non-string-typed key?

wgtmac · 2025-12-11T08:54:03Z

src/iceberg/test/avro_test.cc

+}
+
+// Test both direct decoder and GenericDatum paths
+TEST_F(AvroReaderTest, DirectDecoderVsGenericDatum) {


I would recommend changing it to parameterized test to enable/disable direct decoder for all cases in this file.

wgtmac · 2025-12-11T08:57:31Z

src/iceberg/test/avro_test.cc

+  }
+}
+
+TEST_F(AvroReaderTest, LargeDataset) {


It seems that we don't have test for projection. For example, only some columns are selected and they are reordered.

wgtmac · 2025-12-11T09:05:48Z

BTW, it is possible to add a executable under iceberg/avro folder named avro_scan? We can use it to benchmark on a local avro file and be able to config whether to skip datum or not.

shangxinli force-pushed the avro_reader branch 6 times, most recently from eae14b1 to e7b55b2 Compare December 1, 2025 00:52

shangxinli force-pushed the avro_reader branch from e7b55b2 to e54929d Compare December 1, 2025 00:56

wgtmac reviewed Dec 1, 2025

View reviewed changes

shangxinli force-pushed the avro_reader branch 2 times, most recently from 2334ea3 to d16b0f1 Compare December 1, 2025 03:17

shangxinli force-pushed the avro_reader branch 2 times, most recently from 043bb08 to eae860e Compare December 3, 2025 15:22

shangxinli force-pushed the avro_reader branch from eae860e to 4ed80b0 Compare December 3, 2025 23:49

Smith-Cruise reviewed Dec 9, 2025

View reviewed changes

fix: call decoder.decodeNull() for AVRO_NULL in SkipAvroValue

96b0521

Address reviewer comment to explicitly call decoder.decodeNull() even though AVRO_NULL has no data to skip. This is more consistent with other type handlers and makes the decoder state handling explicit.

shangxinli force-pushed the avro_reader branch from bf67046 to 96b0521 Compare December 9, 2025 17:50

wgtmac reviewed Dec 11, 2025

View reviewed changes

feat: eliminate GenericDatum in Avro reader for performance #374

Are you sure you want to change the base?

feat: eliminate GenericDatum in Avro reader for performance #374

Conversation

shangxinli commented Nov 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants