Skip to content

perf: reuse Variant reconstruction buffers - #6342

Open
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:perf/variant-buffer-reuse
Open

peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:perf/variant-buffer-reuse

Conversation

@peterxcli

Copy link
Copy Markdown
Member

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 CometVariantProjectionSuite tests, 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 optimized ci profile 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:

Input Main bytes/row PR bytes/row Reduction
Canonical 10,332 10,332 0%
Partially shredded 57,402 46,964 18.2%
Fully shredded 52,320 41,882 20.0%
Empty key 88,760 78,375 11.7%

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 1024 consumes 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:

Input Main PR Change Vanilla Spark
Canonical 143 146.5 +2.4% 143.5
Partially shredded 298 271.5 -8.9% 165
Fully shredded 255 268.5 +5.3% 166.5
Empty key 385 373.5 -3.0% 159.5

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.

@github-actions github-actions Bot added enhancement New feature or request performance area:scan Parquet scan / data reading labels Sep 28, 2026
@peterxcli
peterxcli marked this pull request as ready for review September 29, 2026 03:26

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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_variant records checked i32 offsets, 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.

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

Labels

area:scan Parquet scan / data reading enhancement New feature or request performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants