etseidl commented on code in PR #11051:
URL: https://github.com/apache/arrow-rs/pull/11051#discussion_r3982358377
##########
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:
I think the documentation for this needs beefing up. A pointer back to the
parquet documentation of the encoding would be nice. We should also explain why
a user would want to muck with this at all. When would you want settings other
than the defaults?
##########
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:
Why not just add the `&ResolvedColumnOptions` to `get_encoder`? I could see
wanting to tune other encoders. In particular, why not also allow for tuning
the `DeltaBitPackEncoder` used by the other delta encoders?
##########
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:
I don't think we should panic in a fallible function. I think either make
this function infallible, or duplicate the assert logic and return an error.
##########
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:
I'm waffling on this new struct, but on balance I think it's a good thing.
It encapsulates the validation in one place, rather than having separate
properties for block size and miniblocks per block and having to validate when
either is set.
##########
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:
Also an instance where it might be better to pass the column properties
directly rather than resolving the `DeltaBinaryPackedEncoderOptions` beforehand.
##########
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:
The docs here should refer to the `DeltaBinaryPackedEncoderOptions` for an
explanation of why this is ever desirable.
--
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]
