Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Shredded Variant reconstruction allocated per-row value and metadata buffers, then copied their contents into batch buffers.
- Design approach: Reuse batch-scoped builders and construct the final Binary arrays directly from their buffers and offsets.
- Correctness / compatibility analysis: No introduced P1/P2 issues found within this review. Checked dictionary resets, relative offsets, null propagation, slices and metadata flags against Arrow/Parquet 59.3.0 and Spark 4.0.4, 4.1.3 and 4.2.0 reconstruction sources. Local validation passed all 16 Variant tests and 72 base/head comparisons covering changing dictionaries, nested values, Unicode keys, large values and three binary storage formats.
- Key design decisions: Retains the existing reconstruction algorithm and owned Arrow buffers. Builder reuse stays within one batch, with no new shared state or ownership protocol. This reduces allocation and copying work without changing Spark memory-pool accounting.
- Implementation sketch:
rebuild_spark_variantrecords checkedi32offsets, resets dictionary state between rows, clears each metadata sorted flag and reuses parent validity. Intermediate Binary casts are removed. - Behavioral changes worth calling out: Canonical pass-through remains unchanged, and tested reconstructed bytes match the base. Allocation reduction does not establish a scan speedup. The reported timing results remain inconclusive.
- Suggested improvements: None meeting the P1/P2 evidence bar.
Reviewed the entire two-file diff from e897f8ab45dec8fc27c79339c65eeaa7bd60e198 through b2ddd69288fbcc81897c4ebcbc5be109f0467783. GitHub confirms the PR is not a draft. The snapshot and live discussion endpoints contain no existing reviews or comments.
Routed skills: review-comet-pr, review-comet-memory-pr and review-comet-ffi-pr.
Exact-head CI: 23 successful checks, 14 skipped, none failed or unfinished. Required Checks passed. Logs confirm all 16 native Variant tests and all 13 CometVariantProjectionSuite tests passed. Upstream Spark SQL, macOS and benchmark jobs were skipped.
Validation limits: Local tests compiled exact base/head module sources against cached dependencies in an isolated harness. No clean full-project build, local JVM/upstream Spark SQL matrix, or allocation/timing benchmark was rerun.
Which issue does this PR close?
Related to #5978. This is an allocation improvement; #5978 remains open for single-pass reconstruction and its performance target.
Rationale for this change
Shredded Variant reconstruction creates value and metadata builders for every row, then copies their output into batch buffers. Reuse those buffers across rows to reduce allocation traffic with the current Arrow dependency.
What changes are included in this PR?
Append reconstructed values and metadata directly to batch buffers and build the final Binary arrays from their offsets. Reset dictionary state between rows, preserve parent nulls, and avoid intermediate Binary casts before reconstruction.
How are these changes tested?
Exact value and metadata comparisons cover changing dictionaries, large values, null rows and sliced arrays. The 16 native Variant tests, all 13
CometVariantProjectionSuitetests, and six targeted upstream Spark 4.1.3 Parquet Variant tests pass. The upstream assertions are unchanged, with an additional check confirming native scans are active.Matched builds against
e897f8ab4, on Apple M4 with Rust 1.95, the optimizedciprofile and jemalloc. Five alternating native runs, 4,096 rows with 4 KiB payloads and 30 measured batches per case, gave identical allocation counts in every run:These are allocator bytes requested, not retained memory. Both builds use the same benchmark fixtures, including the updated partially shredded empty-key fixture.
CometVariantReadBenchmark 100000 1024consumes both value and metadata bytes from matched Parquet inputs. Four JVM runs per build alternate build order and run both reader orders twice, using Spark 4.1.3 / JDK 17. Median best scan times in milliseconds:Spark medians pool the eight control runs. Concurrent machine activity makes timing inconclusive: even Spark's fully shredded control ranges from 150 to 265 ms. The allocation reduction is reproducible, but this does not establish a scan speedup or rule out a regression. Shredded reads remain slower than Spark. Keeping this PR in draft until a quieter run resolves the scan regression question.