alamb commented on code in PR #10505:
URL: https://github.com/apache/arrow-rs/pull/10505#discussion_r3714898957
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1030,6 +1042,39 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
}
+ /// Exempt a page's mandatory first value from the data page byte limit,
+ /// when that value alone already exceeds it.
+ ///
+ /// Parquet requires every data page to hold at least one value, so such a
+ /// value cannot be split out no matter how the limit is set. Counting it
+ /// against the limit makes the limit unsatisfiable, and
+ /// `should_add_data_page` then cuts a page after every single value. For
+ /// `DELTA_BYTE_ARRAY` that is destructive rather than merely wasteful:
+ /// each page boundary discards the previous value, so a column of large
+ /// values sharing long prefixes degenerates to `PLAIN` (#10489).
Review Comment:
suggest adding a link to the issue rather than the #10489 style
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1030,6 +1042,39 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
}
+ /// Exempt a page's mandatory first value from the data page byte limit,
+ /// when that value alone already exceeds it.
+ ///
+ /// Parquet requires every data page to hold at least one value, so such a
+ /// value cannot be split out no matter how the limit is set. Counting it
+ /// against the limit makes the limit unsatisfiable, and
+ /// `should_add_data_page` then cuts a page after every single value. For
+ /// `DELTA_BYTE_ARRAY` that is destructive rather than merely wasteful:
Review Comment:
I am not sure about "destructive" here -- the point is that it is very
wasteful to encode values on a single page, right?
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1030,6 +1042,39 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
}
+ /// Exempt a page's mandatory first value from the data page byte limit,
+ /// when that value alone already exceeds it.
+ ///
+ /// Parquet requires every data page to hold at least one value, so such a
+ /// value cannot be split out no matter how the limit is set. Counting it
+ /// against the limit makes the limit unsatisfiable, and
+ /// `should_add_data_page` then cuts a page after every single value. For
+ /// `DELTA_BYTE_ARRAY` that is destructive rather than merely wasteful:
+ /// each page boundary discards the previous value, so a column of large
+ /// values sharing long prefixes degenerates to `PLAIN` (#10489).
+ ///
+ /// Recording the value's encoded size here makes the limit apply to what
Review Comment:
I am not sure what this paragraph is trying to say - it seems like maybe it
is describing an implementation detail (so we could remove it?)
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1030,6 +1042,39 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
}
+ /// Exempt a page's mandatory first value from the data page byte limit,
+ /// when that value alone already exceeds it.
+ ///
+ /// Parquet requires every data page to hold at least one value, so such a
Review Comment:
this is a good explanation
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1030,6 +1042,39 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
}
}
+ /// Exempt a page's mandatory first value from the data page byte limit,
+ /// when that value alone already exceeds it.
+ ///
+ /// Parquet requires every data page to hold at least one value, so such a
+ /// value cannot be split out no matter how the limit is set. Counting it
+ /// against the limit makes the limit unsatisfiable, and
+ /// `should_add_data_page` then cuts a page after every single value. For
+ /// `DELTA_BYTE_ARRAY` that is destructive rather than merely wasteful:
+ /// each page boundary discards the previous value, so a column of large
+ /// values sharing long prefixes degenerates to `PLAIN` (#10489).
+ ///
+ /// Recording the value's encoded size here makes the limit apply to what
+ /// follows it — the bytes we *can* still place elsewhere. Only encodings
+ /// that compress against the preceding value opt in, so `PLAIN` and
+ /// `DELTA_LENGTH_BYTE_ARRAY` keep their tighter one-value page bound.
+ ///
+ /// Known limitation: the caller's trigger keys on a page-opening
Review Comment:
this also seems complicated -- is there any way we can fix the limitation
rather than try and caveat it (in comments that are not likely to get read)?
--
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]
