Repository navigation
Conversation
|
Current-head hosted checks at Please approve these fork runs and apply |
dwsmith1983
left a comment
dwsmith1983
left a comment
There was a problem hiding this comment.
Could the description be shortened a bit? The most useful parts are the rationale, the three bullets under "What changes are included", the before/after result of the new compression test, and the decoder timing table. The details about the validation setup and the request to approve CI would work well as a separate comment on the PR instead.
One question about scope. The integration results table lists the Delta suites on Spark 3.5, 4.0 and 4.1, but as far as I can tell delta-spark writes deletion vectors in the portable format, and this PR changes only the native-format decoding path. If that's right, those suites never run the changed code, and the two unit tests here are the only coverage for it. Do you know which writers produce native-format deletion vectors in practice? Saying so in the description would make it clear which tables benefit from the change.
| fn native_decode_preserves_dense_bitmap_compression() { | ||
| let mut bitmap = RoaringBitmap::new(); | ||
| bitmap.insert_range(0..1_000_000); | ||
| let compressed_size = bitmap.serialized_size(); | ||
| let mut bitmap_bytes = Vec::new(); | ||
| bitmap.serialize_into(&mut bitmap_bytes).unwrap(); | ||
|
|
||
| let mut data = NATIVE_MAGIC.to_le_bytes().to_vec(); | ||
| data.extend(1i32.to_le_bytes()); | ||
| data.extend((bitmap_bytes.len() as i32).to_le_bytes()); | ||
| data.extend(bitmap_bytes); | ||
|
|
||
| let decoded = deserialize_dv_bitmap(&data).unwrap(); | ||
| let (_, decoded_bitmap) = decoded.bitmaps().next().unwrap(); | ||
| assert_eq!(decoded_bitmap.serialized_size(), compressed_size); |
There was a problem hiding this comment.
This pins the container type, which is the point of the PR. Could it also push the decoded treemap through build_access_plan? Something like full_file_access_plan(&[600_000, 600_000], &decoded) with group 0 expected as Skip and group 1 as skip 400_000 then select 200_000, plus assert_eq!(decoded.len(), 1_000_000). Nothing in this file currently drives a run container through the plan builder, and after this change dense native DVs will.
There was a problem hiding this comment.
Extended the test with the plan-builder push: full_file_access_plan(&[600_000, 600_000], &decoded) asserts group 0 is Skip, group 1 is skip 400_000 then select 200_000, and decoded.len() == 1_000_000. That is the first case in the file that drives a run container through build_access_plan. Pushed as 263ad46c8.
| #[test] | ||
| fn native_format_decodes() { | ||
| let values = [0u64, 2, 3, 100, (1u64 << 32) + 7]; | ||
| let values = [0u64, 2, 3, 100, (2u64 << 32) + 7]; |
There was a problem hiding this comment.
Worth making the intent visible here, since this value is what guards the empty-partition skip. Either a short comment ("key 1 is an empty partition, key 2 must keep its index") or an assertion on decoded.bitmaps() yielding keys [0, 2].
There was a problem hiding this comment.
Added the assertion as suggested: native_format_decodes now checks decoded.bitmaps() keys are [0, 2] and names the empty partition in a comment, so the skip is asserted where it is guarded. Pushed as 263ad46c8.
Review on apache#6945 asked for the empty intermediate partition to carry its intent where the value guards it, and for the dense native DV to be driven through build_access_plan, which nothing in this file did before.
|
Validation setup and the CI request, moved out of the description per review. Current-head CI request. The draft checks at Please approve these fork runs and apply Local validation setup. Upstream base
It then ran root-reactor
Scope: these suites write and read portable-format deletion vectors, so they exercise the shared decoding path and confirm no regression there; the only cases that execute the changed native decoder are the two native unit tests. The canceled Spark 3.5 timestamp case explicitly requires Spark 4+ and passes on both 4.x profiles. Rebased onto |
Transfer decoded sub-bitmaps instead of reinserting each deleted row to retain compressed runs and implicit high-partition keys. Closes apache#5656
Review on apache#6945 asked for the empty intermediate partition to carry its intent where the value guards it, and for the dense native DV to be driven through build_access_plan, which nothing in this file did before.
263ad46 to
ae0df6e
Compare
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full two-commit diff from 8a4029ccf9a079385224bef8aca95d561993b221 to ae0df6e9b22689e4d040276b24ab598656188c96. The PR is not a draft. Read existing reviews, conversation comments and inline threads. Routed skills: review-comet-pr and review-comet-memory-pr.
No introduced P1/P2 issues found within this review. No substantiated existing P1/P2 concerns remain unresolved.
Summary
- Prior state and problem: Native-format deletion-vector decoding deserialized compressed bitmaps, then enumerated and reinserted every deleted position. This added cardinality-dependent work and expanded compact run containers.
- Design approach: Retain each decoded nonempty bitmap and transfer it into
RoaringTreemap::from_bitmaps, preserving its compression and partition key. - Correctness: Compared Delta’s Scala serialization and row-position composition with both decoder implementations. Independent tests preserved row positions, cardinality and iterator behavior across dense runs, sparse and alternating patterns, unsigned 32-bit boundaries and empty partitions. Delta-generated native and portable fixtures each matched all 1,000,082 JVM row positions.
- Compatibility analysis: Checked
RoaringBitmapArraysources for Delta 3.3.2, 4.0.1 and 4.3.1, paired with Spark 3.5, 4.0 and 4.1. Their relevant implementations are identical. The native format retains implicit array-index keys. Spark 3.4 and 4.2 Delta contrib support remains outside the existing supported pairing. No protocol or dependency changes are introduced. - Key design decisions: Omitting empty bitmaps without renumbering subsequent keys preserves native-format semantics. Converting the validated nonnegative
i32count tou32is safe. Existing framing and decoding validation remains intact. - Implementation sketch: The decoder accumulates owned
(key, bitmap)pairs instead of callingtreemap.insertper row. Tests assert keys[0, 2], preserved compression and the expected fully deleted and partially deleted row-group selections. - Performance: Independently reproduced 230 serialized bytes at the head versus 131,208 on the base for one million contiguous deletions. The removed reinsertion pass explains the improvement. Access-plan construction still visits deleted rows. Decoder timings, end-to-end speed and peak RSS were not independently measured.
- Design: This is a small, localized use of the existing Roaring API. Parsing, error handling and downstream access-plan responsibilities remain easy to follow. No P1/P2 design issue was identified.
- Abstraction & complexity: No new abstraction is introduced. Bitmap payloads move without cloning, while temporary metadata scales with nonempty partitions. Existing selector reservations and Spark memory-pool behavior are unchanged.
- Behavioral changes worth calling out: Only native-format payloads benefit; portable decoding is unchanged. Latest release branch
branch-1.1lacksdelta_dv.rsand has a build-gate Delta planner stub. Native DV support already exists in this PR’s base, so this PR optimizes that unreleased path without introducing another routing change. - Suggested improvements: No additional issue meets the P1/P2 reporting bar. Both earlier requests, explicit empty-partition keys and run-container access-plan coverage, are implemented at this head.
Exact-head CI: Comet CI and CodeQL report action_required. The label workflow passed. There is no hosted build/test verdict for this head.
Validation: Seven isolated tests passed using the exact extracted base/head decoder functions and pinned Roaring 0.11.5, including fixtures generated by compiling Delta’s Scala serializer. git diff --check passed. The full native test attempt could not resolve cached DataFusion 55.2.0 in offline mode. Full Comet, access-plan and Spark/Delta integration suites were not independently rerun; author-reported results are not treated as independent validation. Project files remain unchanged.
dwsmith1983
left a comment
dwsmith1983
left a comment
There was a problem hiding this comment.
Thanks for applying both suggestions. I ran a few more checks on ae0df6e merged with current main, and everything holds up.
- I compared the new native-format decoder against the one on main, using randomly generated inputs (sparse sets, dense runs, empty partitions between keys, all-empty arrays, a zero count) and malformed ones (every truncation, negative and oversized counts and sizes, random byte flips). The two return the same values on every valid input and fail with the same error on every malformed one.
- I encoded bitmaps with Delta's own
RoaringBitmapArrayin the native format (delta-spark 4.3.1), including gaps between keys and values above 2^32, and the new decoder reads each one exactly. Delta writes one entry per key index and uses empty bitmaps for the gaps, which is the layout the decoder assumes. - Under mutation testing of
deserialize_dv_bitmap, the PR's tests catch every mutant on the lines this PR changes, including dropping the empty-bitmap filter, shifting the key and going back to inserting value by value. Theas u32loop bound behaves the same as the oldas u64, becausecountis a non-negativei32. - fmt, clippy and the
delta_dvtests pass, andCometDeltaNativeScanSuitepasses on Spark 4.1 with-Pdelta, deletion vector tests included.
One optional addition: nothing in delta_dv::tests feeds malformed native bytes, so the length checks earlier in this function are not exercised. A small table test would cover them, with a native magic followed by a short header, a count of -1, a count larger than the entries present, and a negative or oversized size. That code predates this PR, so a follow-up is fine too.
3 participants
Which issue does this PR close?
Closes #5656.
Reintroduces the accepted candidate from dwsmith1983#3 on Apache main after #5365 landed; deleting that fork's base branch closed the earlier PR.
Rationale for this change
The native-format deletion-vector decoder (magic 1681511376) deserializes compressed
RoaringBitmapcontainers, then enumerates every deleted row and reinserts it into aRoaringTreemap. Decoding therefore scales with deletion cardinality and expands compact run containers. On base474b5ece841a7fc6dff37ae04bd6787e654d7be7with Roaring 0.11.5, decoding one contiguous one-million-row sub-bitmap yields 131,208 serialized bytes instead of 230.What changes are included in this PR?
RoaringTreemap::from_bitmaps.build_access_plan, and cover an empty intermediate partition in native-format decoding.Only
native/core/src/execution/delta_dv.rschanges, and only the decoder's per-row reinsertion pass is removed: access-plan construction still iterates deleted rows to build row-group selections, including partially deleted row groups. #5655 is not included.Which tables use the native format?
Every Delta OSS writer emits the portable format (magic 1681511377) instead: Spark passes
RoaringBitmapArrayFormat.PortabletoDeletionVectorUtils.serializeat every deletion-vector write site, and has since DV support was open-sourced (0a0ea97bc6, 2023-10); the spec switched inc7934d3b30(2022-12);PROTOCOL.mddescribes only that payload.NativeRoaringBitmapArraySerializationFormatsurvives on the read path alone, so this decoder is compatibility coverage for deletion-vector files written before the switch, i.e. Databricks Runtime writes predating portable. The Delta suites in the linked comment therefore never execute the changed code, and the two native unit tests here are its only coverage. If a native-emitting writer is known, naming it would make the affected table class explicit.How are these changes tested?
Decoder measurements
Release-mode harness over the base and patched decoder bodies, Roaring 0.11.5, identical rows and cardinalities verified before timing. One native sub-bitmap of one million positions per pattern; the contiguous fixture uses
insert_range.The contiguous fixture decodes to 230 serialized bytes on this patch and 131,208 on the base. Medians of seven 20-decode sample means on an unpinned shared Linux host; allocated bytes are cumulative requested bytes, not peak memory or RSS. Decoder-only, with no access-plan or I/O measurement.