Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve parquet reading performance for columns with nulls by preserving bitmask when possible (#1037) #1054

Merged
merged 6 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions parquet/src/arrow/record_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,22 @@ where
/// If `null_mask_only` is true only the null bitmask will be generated and
/// [`Self::consume_def_levels`] and [`Self::consume_rep_levels`] will always return `None`
///
/// It is insufficient to solely check that that the max definition level is 1 as we
/// need there to be no nullable parent array that will required decoded definition levels
///
/// In particular consider the case of:
///
/// ```ignore
/// message nested {
/// OPTIONAL Group group {
/// REQUIRED INT32 leaf;
/// }
/// }
/// ```
///
/// The maximum definition level of leaf is 1, however, we still need to decode the
/// definition levels so that the parent group can be constructed correctly
///
pub(crate) fn new_with_options(desc: ColumnDescPtr, null_mask_only: bool) -> Self {
let def_levels = (desc.max_def_level() > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the use of null_mask_only here -- I thought null_mask_only would be set only if max_def_level() == )

AKA https://github.com/apache/arrow-rs/pull/1054/files#diff-0d6bed48d78c5a2472b7680a8185cabdc0bd259d6484e184439ed7830060661fR1374

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment clarifying, its an edge case of nested nullability. Perhaps I should add an explicit test 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added in 59846eb

.then(|| DefinitionLevelBuffer::new(&desc, null_mask_only));
Expand Down
18 changes: 18 additions & 0 deletions parquet/src/arrow/record_reader/definition_levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ enum BufferInner {
max_level: i16,
},
/// Only compute null bitmask - requires max level to be 1
tustvold marked this conversation as resolved.
Show resolved Hide resolved
///
/// This is an optimisation for the common case of a nullable scalar column, as decoding
/// the definition level data is only required when decoding nested structures
///
Mask { nulls: BooleanBufferBuilder },
}

Expand Down Expand Up @@ -228,6 +232,20 @@ impl ColumnLevelDecoder for DefinitionLevelDecoder {
}
}

/// An optimized decoder for decoding [RLE] and [BIT_PACKED] data with a bit width of 1
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

/// directly into a bitmask
///
/// This is significantly faster than decoding the data into `[i16]` and then computing
/// a bitmask from this, as not only can it skip this buffer allocation and construction,
/// but it can exploit properties of the encoded data to reduce work further
///
/// In particular:
///
/// * Packed runs are already bitmask encoded and can simply be appended
/// * Runs of 1 or 0 bits can be efficiently appended with byte (or larger) operations
///
/// [RLE]: https://github.com/apache/parquet-format/blob/master/Encodings.md#run-length-encoding--bit-packing-hybrid-rle--3
/// [BIT_PACKED]: https://github.com/apache/parquet-format/blob/master/Encodings.md#bit-packed-deprecated-bit_packed--4
struct PackedDecoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code looks quite similar to BitReader https://github.com/tustvold/arrow-rs/blob/bitmask-preservation/parquet/src/util/bit_util.rs#L501

I wonder if you looked at possibly reusing that implmentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer is not using that implementation is the major reason this PR represents a non-trivial speed bump, it can decode more optimally as it can decode directly using append_packed_range / append_n. Will add some comments clarifying

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct PackedDecoder {
/// Specialized decoder for bitpacked hybrid format (TODO link) that contains
/// only 0 and 1 (for example, definition levels in a non-nested column)
/// that directly decodes into a bitmask in the fastest possible way
struct PackedDecoder {

Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to leave breadcrumbs for the next person to look at this code. Is this a correct description of what this structure implements?

data: ByteBufferPtr,
data_offset: usize,
Expand Down