feat: eliminate GenericDatum in Avro reader for performance #374
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shangxinli
force-pushed
the
avro_reader
branch
6 times, most recently
from
eae14b1 to
e7b55b2
Compare
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
shangxinli
force-pushed
the
avro_reader
branch
from
e7b55b2 to
e54929d
Compare
wgtmac
reviewed
Dec 1, 2025
src/iceberg/avro/avro_reader.cc
Outdated
| ICEBERG_RETURN_UNEXPECTED( | ||
| AppendDatumToBuilder(reader_->readerSchema().root(), *context_->datum_, | ||
| projection_, *read_schema_, context_->builder_.get())); | ||
| DecodeAvroToBuilder(reader_->readerSchema().root(), reader_->decoder(), |
Member
There was a problem hiding this comment.
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.
shangxinli
force-pushed
the
avro_reader
branch
2 times, most recently
from
2334ea3 to
d16b0f1
Compare
Member
|
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 |
shangxinli
force-pushed
the
avro_reader
branch
2 times, most recently
from
043bb08 to
eae860e
Compare
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.
shangxinli
force-pushed
the
avro_reader
branch
from
eae860e to
4ed80b0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace GenericDatum intermediate layer with direct Avro decoder access to improve manifest I/O performance.
Changes:
Performance improvement:
This matches Java implementation which uses Decoder directly via ValueReader interface, avoiding intermediate object allocation.
All avro_test cases pass.
Issue: #332