emecii commented on code in PR #11051:
URL: https://github.com/apache/arrow-rs/pull/11051#discussion_r3982674725
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -344,6 +357,54 @@ const MAX_PAGE_HEADER_WRITER_SIZE: usize = 32;
const DEFAULT_BIT_WRITER_SIZE: usize = 1024 * 1024;
const DEFAULT_NUM_MINI_BLOCKS: usize = 4;
+/// Controls the block layout emitted by [`DeltaBitPackEncoder`].
+///
+/// The Parquet format requires block sizes to be multiples of 128 and mini
+/// block sizes to be multiples of 32.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct DeltaBinaryPackedEncoderOptions {
Review Comment:
Kept the options struct and its centralized `try_new` validation, while
making the encoder constructor infallible because it only accepts a validated
instance.
##########
parquet/src/file/properties.rs:
##########
@@ -1064,6 +1079,16 @@ impl WriterPropertiesBuilder {
self
}
+ /// Sets the default delta binary packed encoder block layout for all
columns.
Review Comment:
Done. The getter and both builder setters now point to
`DeltaBinaryPackedEncoderOptions` for the trade-off explanation and note that
the setting also controls the integer sub-encoders used by the two byte-array
delta encodings.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -90,6 +90,19 @@ pub fn get_encoder<T: DataType>(
<T::T as private::GetEncoder>::get_encoder(descr, encoding)
}
+pub(crate) fn get_encoder_with_options<T: DataType>(
Review Comment:
Done. The column writer now passes `&ResolvedColumnProperties` to
`get_encoder_with_properties` while preserving the existing public
`get_encoder` signature. The selected layout is applied to
`DELTA_BINARY_PACKED`, `DELTA_LENGTH_BYTE_ARRAY`, and `DELTA_BYTE_ARRAY`.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -403,21 +464,34 @@ impl<T: DataType> DeltaBitPackEncoder<T> {
let block_size = mini_block_size * num_mini_blocks;
assert_eq!(block_size % 128, 0);
+ Self::new_with_layout(block_size, num_mini_blocks)
+ }
+
+ fn new_with_layout(block_size: usize, num_mini_blocks: usize) -> Self {
DeltaBitPackEncoder {
page_header_writer: BitWriter::new(MAX_PAGE_HEADER_WRITER_SIZE),
bit_writer: BitWriter::new(DEFAULT_BIT_WRITER_SIZE),
total_values: 0,
first_value: 0,
current_value: 0, // current value to keep adding deltas
block_size, // can write fewer values than block size for
last block
- mini_block_size,
+ mini_block_size: block_size / num_mini_blocks,
num_mini_blocks,
values_in_block: 0, // will be at most block_size
deltas: vec![0; block_size],
_phantom: PhantomData,
}
}
+ /// Creates a delta bit packed encoder with a custom block layout.
+ pub fn try_new_with_options(options: DeltaBinaryPackedEncoderOptions) ->
Result<Self> {
Review Comment:
Done. Resolution remains in `WriterProperties`, and `ColumnValueEncoder` now
passes the already resolved column properties directly into encoder selection.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -403,21 +464,34 @@ impl<T: DataType> DeltaBitPackEncoder<T> {
let block_size = mini_block_size * num_mini_blocks;
assert_eq!(block_size % 128, 0);
+ Self::new_with_layout(block_size, num_mini_blocks)
+ }
+
+ fn new_with_layout(block_size: usize, num_mini_blocks: usize) -> Self {
DeltaBitPackEncoder {
page_header_writer: BitWriter::new(MAX_PAGE_HEADER_WRITER_SIZE),
bit_writer: BitWriter::new(DEFAULT_BIT_WRITER_SIZE),
total_values: 0,
first_value: 0,
current_value: 0, // current value to keep adding deltas
block_size, // can write fewer values than block size for
last block
- mini_block_size,
+ mini_block_size: block_size / num_mini_blocks,
num_mini_blocks,
values_in_block: 0, // will be at most block_size
deltas: vec![0; block_size],
_phantom: PhantomData,
}
}
+ /// Creates a delta bit packed encoder with a custom block layout.
+ pub fn try_new_with_options(options: DeltaBinaryPackedEncoderOptions) ->
Result<Self> {
+ Self::assert_supported_type();
Review Comment:
Made this infallible as `new_with_options`.
`DeltaBinaryPackedEncoderOptions` is validated at construction, so this
constructor no longer exposes a fallible path that can panic.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -344,6 +357,54 @@ const MAX_PAGE_HEADER_WRITER_SIZE: usize = 32;
const DEFAULT_BIT_WRITER_SIZE: usize = 1024 * 1024;
const DEFAULT_NUM_MINI_BLOCKS: usize = 4;
+/// Controls the block layout emitted by [`DeltaBitPackEncoder`].
+///
+/// The Parquet format requires block sizes to be multiples of 128 and mini
Review Comment:
Expanded the public docs with the Parquet `DELTA_BINARY_PACKED`
specification link, the type-specific defaults, and concrete guidance for
stable versus locally varying delta widths, padding, metadata, throughput, and
scratch-memory trade-offs. I also added a reproducible Criterion benchmark with
encoded sizes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
