cakeni commented on code in PR #10777:
URL: https://github.com/apache/arrow-rs/pull/10777#discussion_r3857968672
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1079,14 +1084,34 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
/// Performs dictionary fallback.
- /// Prepares and writes dictionary and all data pages into page writer.
+ ///
+ /// The values buffered for the in-progress data page are dictionary ids;
+ /// they are re-encoded through the fallback encoder (together with the
+ /// dictionary they resolve to) rather than flushed as one more
+ /// dictionary-encoded page, and remain buffered. This matches
+ /// parquet-java's `FallbackValuesWriter`.
+ ///
+ /// If dictionary-encoded data pages have already been produced for this
+ /// chunk they reference the dictionary, so the dictionary page must still
+ /// be written, ahead of any data pages buffered while awaiting it.
+ /// Otherwise no dictionary page is written at all and the whole chunk uses
+ /// the fallback encoding.
fn dict_fallback(&mut self) -> Result<()> {
// At this point we know that we need to fall back.
- if self.page_metrics.num_buffered_values > 0 {
+ let retain_dictionary = self.has_dictionary_encoded_data_pages;
+ self.encoder.fall_back_from_dictionary(retain_dictionary)?;
+ if retain_dictionary {
+ self.write_dictionary_page()?;
+ while let Some(page) = self.data_pages.pop_front() {
+ self.write_data_page(page)?;
+ }
+ }
+ // The dictionary ids held only a fraction of the page size budget; the
+ // same values re-encoded with the fallback encoding may already exceed
+ // it.
+ if self.should_add_data_page() {
Review Comment:
Could this let fallback bypass the data-page size limit?
fall_back_from_dictionary re-encodes the entire buffered dictionary-id buffer
before should_add_data_page() runs. Since add_data_page() then flushes the
buffer as a whole, it can’t split it back into page-sized chunks.
With 1 KiB dictionary/data page limits and write_batch_size = 1, I get a
41,612-byte PLAIN page with this PR, while the parent keeps pages under 4 KiB.
Should we re-encode the buffered ids incrementally during fallback so the page
limit is still enforced?
--
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]
