Repository navigation
perf(parquet): build list offsets and validity without branching on levels - #12018
Open
dwsmith1983 wants to merge 3 commits into
Open
dwsmith1983 wants to merge 3 commits into
dwsmith1983 wants to merge 3 commits into
Conversation
Adds ListArray/Int32ListList (no NULLs, half NULLs) to the arrow_reader bench. The reader is built the way builder.rs builds it for a list<list<int32>> column, so the outer list runs with a parent threshold and the nested path in ListArrayReader::consume_batch is measured.
…evels ListArrayReader::consume_batch walked the def and rep levels with a match per level, which mispredicts often when NULLs and empty lists are mixed in. The scan now computes each level's contribution with boolean arithmetic: offsets are written by index into a pre-sized buffer, and validity bits are packed into a u64 and flushed with append_word. The loop is specialized on whether a parent threshold applies and whether the list is nullable. A list offset that overflows the offset type now returns an error instead of panicking. Tests compare the new scan against a copy of the old loop on seeded random levels (depths 1 to 3, lengths around word boundaries, malformed input, i32 and i64 offsets), and round-trip list<list> and list in struct at a small batch size.
Author
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of apache/datafusion-comet#6921.
Rationale for this change
ListArrayReader::consume_batchbuilds list offsets and validity with a branch per level, and those branches mispredict whenever NULLs, empty lists and values are mixed. Nested lists pay it again at every level.What changes are included in this PR?
u64and appended withappend_word.ListArray/Int32ListListbench (no NULLs, half NULLs) covers the nested path. It is in its own commit, so the change can be compared against it.Are these changes tested?
A differential test compares the new scan with a copy of the old loop on seeded random levels: nesting depth 1 to 3, lengths around the 64-bit word boundaries, malformed input, and both
i32andi64offsets. Two round-trip tests readlist<list<int32>>and a list inside a struct at a batch size of 7.Benchmarks ran on GitHub-hosted runners: the bench commit and this change, each built once, then run in alternating order. Change in time, negative is faster:
StringList/no NULLsStringList/half NULLsStringList/90pct NULLsStringList/99pct NULLsInt32List/no NULLsInt32List/half NULLsInt32List/90pct NULLsInt32List/99pct NULLsInt32List/no NULLs or empty listsInt32List/no NULLs, skip and read in runs of 1Int32List/no NULLs, skip and read in runs of 32Int32ListList/no NULLsInt32ListList/half NULLsFixed32List/no NULLsFixed32List/half NULLsFixed32List/90pct NULLsFixed32List/99pct NULLsRunning the two binaries in the opposite order gives the same picture on both machines.
skip and read in runs of 1moved between -2% and +3% across runs on x86. That bench callsconsume_batchonly five times per iteration, about 5% of its instructions, and callgrind shows those calls run the same instruction count as before with far fewer mispredicts.Are there any significant user-facing changes?
No.