adriangb commented on code in PR #9972:
URL: https://github.com/apache/arrow-rs/pull/9972#discussion_r3352026565
##########
parquet/src/column/writer/encoder.rs:
##########
@@ -411,3 +507,38 @@ where
}
}
}
+
+/// Plain-encoded byte cost of a single value of type `T::T`.
+///
+/// Derived from [`ParquetValueType::dict_encoding_size`] so we don't add a
+/// parallel per-value-size hook to the trait. The components returned by
+/// `dict_encoding_size` are `(per-value overhead, value-bytes)`. For
+/// plain encoding the on-disk layout is:
+///
+/// - `BYTE_ARRAY`: 4-byte length prefix + payload bytes = `overhead + bytes`.
Review Comment:
Moved the BYTE_ARRAY / FLBA / numeric rationale to inline `//` comments on
the match arms; kept the benchmark-placement note. Resolved in 2acb3f2060.
##########
parquet/src/column/writer/mod.rs:
##########
@@ -713,6 +760,78 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E>
{
})
}
+ /// Writes a chunk in sub-batches sized by the caller-supplied
+ /// `sub_batch_size` so the post-write data page byte limit check
+ /// fires before the chunk can grossly overshoot
+ /// `data_page_size_limit`.
+ ///
Review Comment:
Reworded close to your suggestion. Resolved in 2acb3f2060.
##########
parquet/src/column/writer/mod.rs:
##########
@@ -713,6 +760,78 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E>
{
})
}
+ /// Writes a chunk in sub-batches sized by the caller-supplied
+ /// `sub_batch_size` so the post-write data page byte limit check
+ /// fires before the chunk can grossly overshoot
+ /// `data_page_size_limit`.
+ ///
+ /// For flat (unrepeated) columns sub-batches contain up to
+ /// `sub_batch_size` levels each. For repeated/nested columns
+ /// sub-batches step from one `rep == 0` boundary to the next so a
+ /// record never spans data pages, matching the parquet format rule.
+ ///
+ /// Returns the total number of values consumed across all sub-batches.
+ ///
+ /// `#[inline(never)]` keeps this slow path — only reached for
+ /// variable-width columns whose values need page splitting — out of
+ /// the hot `write_batch_internal` loop.
+ #[allow(clippy::too_many_arguments)]
+ #[inline(never)]
+ fn write_granular_chunk(
+ &mut self,
+ values: &E::Values,
+ values_offset: usize,
+ value_indices: Option<&[usize]>,
+ chunk_size: usize,
+ chunk_def: LevelDataRef<'_>,
+ chunk_rep: LevelDataRef<'_>,
+ sub_batch_size: usize,
+ ) -> Result<usize> {
+ let mut values_consumed = 0;
+ let mut sub_start = 0;
+ while sub_start < chunk_size {
+ let sub_end = match chunk_rep {
+ LevelDataRef::Materialized(levels) => {
+ // Pack up to `sub_batch_size` levels per mini-batch, then
+ // extend to the next record boundary (rep == 0) so a
+ // record never spans data pages. Packing whole records
+ // rather than stepping one record at a time avoids
+ // emitting a `write_mini_batch` per record: records
Review Comment:
Applied. Resolved in 2acb3f2060.
##########
parquet/src/column/writer/mod.rs:
##########
@@ -713,6 +760,78 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E>
{
})
}
+ /// Writes a chunk in sub-batches sized by the caller-supplied
+ /// `sub_batch_size` so the post-write data page byte limit check
+ /// fires before the chunk can grossly overshoot
+ /// `data_page_size_limit`.
+ ///
+ /// For flat (unrepeated) columns sub-batches contain up to
+ /// `sub_batch_size` levels each. For repeated/nested columns
+ /// sub-batches step from one `rep == 0` boundary to the next so a
+ /// record never spans data pages, matching the parquet format rule.
+ ///
+ /// Returns the total number of values consumed across all sub-batches.
+ ///
+ /// `#[inline(never)]` keeps this slow path — only reached for
+ /// variable-width columns whose values need page splitting — out of
+ /// the hot `write_batch_internal` loop.
+ #[allow(clippy::too_many_arguments)]
+ #[inline(never)]
+ fn write_granular_chunk(
+ &mut self,
+ values: &E::Values,
+ values_offset: usize,
+ value_indices: Option<&[usize]>,
+ chunk_size: usize,
+ chunk_def: LevelDataRef<'_>,
+ chunk_rep: LevelDataRef<'_>,
+ sub_batch_size: usize,
+ ) -> Result<usize> {
+ let mut values_consumed = 0;
+ let mut sub_start = 0;
+ while sub_start < chunk_size {
+ let sub_end = match chunk_rep {
+ LevelDataRef::Materialized(levels) => {
+ // Pack up to `sub_batch_size` levels per mini-batch, then
+ // extend to the next record boundary (rep == 0) so a
+ // record never spans data pages. Packing whole records
+ // rather than stepping one record at a time avoids
+ // emitting a `write_mini_batch` per record: records
+ // average only a handful of levels, so the
Review Comment:
Reworded so it reads as a forward rationale (why we pack whole records)
rather than describing a previous implementation. Resolved in 2acb3f2060.
##########
parquet/src/column/writer/byte_budget_chunker.rs:
##########
@@ -0,0 +1,206 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Decides how many levels of a chunk to write as one mini-batch so that
Review Comment:
Moved the background onto `ByteBudgetChunker`; the module doc now just links
to it. Resolved in 2acb3f2060.
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -593,6 +679,142 @@ where
}
}
+/// Cumulative-scan fallback used for byte array types that don't expose
Review Comment:
This turned out to be unreachable dead code — a bare `FixedSizeBinary` goes
to the generic column writer, and `Dictionary(FixedSizeBinary)` hits the
`Dictionary(_, _)` arm — so the whole function and its dispatch arm were
removed in 3a7161fab7. Resolved.
--
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]
