Repository navigation
[common] Cache decoded variant metadata dictionary during rebuild - #10226
juntaozhang wants to merge 5 commits into
Conversation
e8e1547 to
37857ac
Compare
|
Requirement fit: SUPPORTED. Implementation: FINDINGS at [P2] Adopt the dictionary once per top-level reconstruction
I benchmarked actual
A separate fork reproduced approximately 2.5×/9.3×/20.9× slowdowns. These are reconstruction timings, not claimed storage-throughput measurements. Keep the cross-row cache, but compare/adopt at most once after each [P2] Compare direct buffers independently of their byte order
I reproduced wrong output through the existing public Both contain one valid UTF-8 key. Warm the reader with A, then provide B in a direct buffer with its default byte order: the shared reader returns A's field name, while a fresh reader and the base implementation return B's correct field name. The rows were created through Validation: on both JDK 8 and JDK 11, the relevant common/serializer and real Parquet shredding write/read suites executed 106 tests (105 passed, 1 skipped) with normal Maven checks. Current Java/integration CI jobs were cancelled, so completed green integration results are still needed after fixing the regressions. |
cce4c0f to
388d225
Compare
|
[P2] Adopt the dictionary once per top-level reconstruction.
@JingsongLi Thx for the review! Both P2s fixed in 388d225. CI green. PTAL? |
JingsongLi
left a comment
There was a problem hiding this comment.
Dictionary reuse has end-to-end value. On this head, the 93 relevant Common/serializer/real Parquet shredding tests executed successfully on both JDK 8 and JDK 11 (92 passed, one skipped per run), with standard Maven checks; exact-head CI is green. The prior direct-buffer byte-order error and recursive whole-value rebuild problem are fixed. Additional production-class probes reveal a remaining projected-array performance path and a narrower public-reader error-recovery regression below. No production storage or credentials were used, and the worktree source remains unchanged.
| /** A util function to rebuild the variant in binary format from a variant value. */ | ||
| protected Variant rebuildVariant(InternalRow row, ByteBuffer topLevelMetadata) { | ||
| GenericVariantBuilder builder = new GenericVariantBuilder(false); | ||
| cachedMetadata.adopt(topLevelMetadata); |
There was a problem hiding this comment.
[P2] Share the adopted dictionary across projected array elements
The recursive ShreddingUtils fix does not cover projected reads through ArrayReader: its element loop repeatedly invokes the same child VariantReader, and every child rebuild reaches this full-buffer comparison. For a shredded object with items: ARRAY<ROW<typed: INT>> and one distinct leftover_i field per element, projecting $.items as ARRAY performs N scans of an O(N)-sized metadata dictionary. I benchmarked the actual buildFieldsToExtract + assembleVariantStruct production path against baseline source classes on JDK 8/G1. In the second independent JVM fork, median baseline/head times were 534/835 us at 1,000 elements, 2,490/13,309 us at 5,000, and 3,669/47,043 us at 10,000; the first fork reproduced approximately 1.45x/5.48x/14.58x slowdowns. Canonical per-element output, SHA-256, and retained results after a different-metadata read all matched. Please adopt once per outer row/read and share that adopted dictionary with the child readers. A buffer-identity-only shortcut would lose the backing-array reuse protection, so include projected-array coverage as well as full-value reconstruction.
| return; | ||
| } | ||
|
|
||
| reuse = snapshot(current); |
There was a problem hiding this comment.
[P2] Publish cache state only after metadata validation succeeds
This updates reuse and then dictSize before validating the offset table. If validation throws, an identical subsequent buffer hits the equality return while decoded entries from the previous valid row remain in dict. Through the public BaseVariantReader API, warming on {"a":1} and then reading a {"b":2} value with truncated metadata [1,1] first throws MALFORMED_VARIANT, but the identical second read silently returns {"a":2}. A fresh reader and the baseline reject both reads; a control that validates candidate state before publishing it also rejects both. This requires a caller that catches malformed-row errors and reuses the reader/cache; the built-in Parquet batch path aborts on the first error. Please commit the new snapshot/header/dictionary state only after validation, or invalidate all cached state on failure, and verify repeated bad-row reads after a valid dictionary warmup.
There was a problem hiding this comment.
Thanks for the detailed review. I've reworked the metadata-dictionary caching to address both points.
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed the current head in an isolated worktree. The two previous findings (repeated projected-array metadata adoption and cache poisoning after failed header validation) are fixed. The no-fast-build JDK 8 Common/Parquet run executed 100 relevant cases: 99 passed and one skipped, including real Parquet shredding reads and writes. Standard formatting/checkstyle checks passed. A new two-field probe using the existing four-argument helper fails under the same standard checks, as described below. The latest functional Java CI jobs passed; the licensing lane fails on the unchanged Python BTree resources README header, outside this diff.
| } | ||
|
|
||
| VariantMetadata metadata = fields[0].reader.metadata(); | ||
| metadata.adopt(topLevelMetadata); |
There was a problem hiding this comment.
[P2] Initialize each distinct cache used by the supplied field extractors
The retained four-argument buildFieldsToExtract overload creates a fresh VariantMetadata per call, but this now adopts metadata only for fields[0]. Previously valid code that builds two extractors with separate ordinary calls (for example metadata must be adopted before read), instead of returning both fields; an unshredded fallback can dereference a null buffer. The baseline tests used this construction and this PR changes them to explicit shared caches, hiding the compatibility break. The standard Parquet getFieldsToExtract path shares a cache correctly, so this is specifically a regression in the existing public helper combination. Please adopt once per distinct cache in the supplied fields, or otherwise preserve the four-argument helper contract while keeping shared/recursive readers on the single-adoption path. Add a multi-field test using the retained overload.
|
Hi @JingsongLi, can you help cancel the stuck queued run https://github.com/apache/paimon/actions/runs/38422757429 |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS. The previous byte-order, recursive/projected-array adoption, malformed-header recovery and distinct-helper-cache issues are repaired. One additional real Parquet projection performance regression remains below. Normal JDK 8 checks passed 93 relevant Common/serializer/Parquet tests (one existing disabled case); the same 93 passed on JDK 11. Whole-value and projected-array reconstruction results match the exact merge-base, and the previous quadratic path is gone. The current head-associated CI is green.
| for (FieldToExtract field : fields) { | ||
| VariantMetadata metadata = field.reader().metadata(); | ||
| if (adopted.add(metadata)) { | ||
| metadata.adopt(topLevelMetadata); |
There was a problem hiding this comment.
[P2] Adopt metadata lazily for fully typed projections
This now compares the entire metadata buffer for every row even when all requested fields are already present in typed_value. In the baseline, projecting a non-null shredded INT goes straight to ScalarReader.getInt; it does not parse or compare the unrelated dictionary. The Parquet pruner retains the real metadata column for possible fallback, so this is also reached by ordinary projected file reads through assembleVariantStructBatch.
I reproduced this with actual ParquetFileFormat shredding writes and projected reads, using the same physical files and checking all 5,000 returned INT values and their checksum. With 10,000 unprojected leftover keys (158,905 metadata bytes), reading only $.typed as INT takes a median 25.21 ms on this head versus 2.31 ms on the exact merge-base 10cf6eb (JDK 8/G1, warm local files, separate JVMs; seven measured scans after warmup). With 1,000 keys it is 6.19 vs 3.89 ms. Both files return 5,000 rows and sum 615,000. Reversing JVM execution order still gives 24.40 ms on HEAD versus 2.76 ms on the baseline for the 10,000-key file (8.84x). A separate production-helper size sweep also confirms the new per-row cost grows with dictionary size.
Please defer adoption until the current row first actually needs metadata for fallback/rebuild/dictionary access, sharing that adoption across recursive readers. Keep dynamic fallback for rows without a typed value and backing-buffer reuse detection; a schema-only or buffer-identity shortcut would be unsafe. Add a fully typed narrow-projection case with large unprojected metadata so the cache optimization does not make that read linear in unrelated dictionary size.
Purpose
Optimize variant metadata decoding by caching a batch's (usually identical) metadata bytes in a
single-slot
VariantMetadata, so the dictionary is parsed only once per distinct buffer instead ofon every row.
Tests
Benchmark: juntaozhang@07b0d2f
Compares the optimized path (WITH cache) against the baseline (WO, no cache) on
OBJECT_LEFT_L64, R=500k, G1 6g. All points show only minor GC, 0 MAJOR , median of 2 runs, JDK 8,11, theVariantMetadatacache speeds up variant metadata decoding by roughly ~1.0–1.3x on both JDK 8 and JDK 11 :