Skip to content

[common] Cache decoded variant metadata dictionary during rebuild - #10226

Open
juntaozhang wants to merge 5 commits into
apache:masterfrom
juntaozhang:pr-variant-metadata-cache
Open

juntaozhang wants to merge 5 commits into
apache:masterfrom
juntaozhang:pr-variant-metadata-cache

Conversation

@juntaozhang

Copy link
Copy Markdown
Contributor

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 of
on 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, the VariantMetadata cache speeds up variant metadata decoding by roughly ~1.0–1.3x on both JDK 8 and JDK 11 :

reuse WO ns (JDK 8) WITH ns (JDK 8) spd JDK 8 WO ns (JDK 11) WITH ns (JDK 11) spd JDK 11
0% 12868.2 12584.5 1.02x 10700.7 10741.7 1.00x
10% 11030.9 10560.4 1.04x 8744.2 9488.3 0.92x
20% 10997.8 10322.8 1.07x 8761.3 8764.5 1.00x
30% 10773.0 10114.2 1.07x 8794.5 8519.2 1.03x
40% 10455.6 9710.9 1.08x 8464.6 8224.8 1.03x
50% 10268.3 9424.4 1.09x 8267.6 7896.8 1.05x
60% 10056.1 9098.1 1.11x 8062.6 7510.4 1.07x
70% 9844.0 8721.1 1.13x 7745.0 7184.2 1.08x
80% 9627.2 8276.0 1.16x 7541.3 6780.3 1.11x
90% 9561.5 7815.1 1.22x 7288.7 6252.8 1.17x
100% 12040.9 9371.0 1.29x 9163.1 7428.6 1.23x

@juntaozhang
juntaozhang force-pushed the pr-variant-metadata-cache branch from e8e1547 to 37857ac Compare September 27, 2026 02:55
@JingsongLi

Copy link
Copy Markdown
Contributor

Requirement fit: SUPPORTED. Implementation: FINDINGS at 37857ace36. Reusing decoded dictionaries has value, but these two regressions should be fixed before merging.

[P2] Adopt the dictionary once per top-level reconstruction

ShreddingUtils.java:201 calls cachedMetadata.adopt() for every partially shredded nested object. Each call compares the entire top-level metadata buffer. With an array of N objects having distinct leftover keys, metadata grows with N, so rebuilding one value now scans O(N²) bytes. This affects ordinary full-variant/JSON reconstruction through BaseVariantReader and assembleVariantBatch, even when every row shares metadata.

I benchmarked actual PaimonShreddingUtils.assembleVariant against the exact base implementation on JDK 8/G1. The schema is {items: array<Row<typed:int>>}, and each input element contains typed plus a distinct leftover_i field. Base/head outputs have identical canonical JSON and SHA-256. After warmup, median reconstruction times in a second JVM fork were:

Elements Base This PR
1,000 233 µs 582 µs
5,000 1,448 µs 11,647 µs
10,000 2,393 µs 46,520 µs

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 setCurrent(); preserve detection of reused backing arrays between rows.

[P2] Compare direct buffers independently of their byte order

BinaryRowDataUtil.java:125-126 compares numeric getLong() results using each buffer's byte order. A little-endian snapshot and a default big-endian direct buffer with reversed eight-byte chunks can therefore compare equal despite different bytes, causing stale dictionary keys to be used.

I reproduced wrong output through the existing public BaseVariantReader.read(row, ByteBuffer) API using valid canonical 16-byte metadata:

A = [1,1,0,12,12,0,1,1,97,98,99,100,101,102,103,104]
B = [1,1,0,12,12,0,1,1,104,103,102,101,100,99,98,97]

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 castShredded using an empty object shredding schema; no truncated or padded metadata is involved. Internal vector callers explicitly use little endian, so this is the narrower public ByteBuffer path. Compare raw bytes or normalize duplicate buffers to the same order without mutating caller state, and cover both false matches and false misses.

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.

@juntaozhang
juntaozhang force-pushed the pr-variant-metadata-cache branch from cce4c0f to 388d225 Compare October 11, 2026 14:06
@juntaozhang

juntaozhang commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor Author

[P2] Adopt the dictionary once per top-level reconstruction.
Re-ran the element sweep on the fix branch to confirm, on JDK 8 / G1, real ShreddingUtils.rebuild path:

N base this PR (fixed)
1,000 337 µs 286 µs
2,000 781 µs 562 µs
5,000 1,488 µs 1,290 µs
10,000 3,023 µs 2,735 µs

@JingsongLi Thx for the review! Both P2s fixed in 388d225. CI green. PTAL?

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review. I've reworked the metadata-dictionary caching to address both points.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 $.a as INT and $.b as STRING) and passes them together to assembleVariantStruct leaves the second cache empty. A real castShredded/assembleVariantStruct probe with {"a":1,"b":"hello"} now throws IllegalStateException (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.

@juntaozhang juntaozhang reopened this Oct 13, 2026
@juntaozhang

Copy link
Copy Markdown
Contributor Author

Hi @JingsongLi, can you help cancel the stuck queued run https://github.com/apache/paimon/actions/runs/38422757429
It's wedged in queued and holding the ci-pull_request-10226 concurrency group, blocking PR #10226 from re-running. Thanks!

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants