adriangb commented on code in PR #25940:
URL: https://github.com/apache/datafusion/pull/25940#discussion_r4168146389
##########
datafusion/proto-common/src/generated/pbjson.rs:
##########
@@ -2600,7 +2600,7 @@ impl<'de> serde::Deserialize<'de> for CsvWriterOptions {
if compression_level__.is_some() {
return
Err(serde::de::Error::duplicate_field("compressionLevel"));
}
- compression_level__ =
+ compression_level__ =
Review Comment:
This change (and the ones at `terminator__` and `.copied()` → `.cloned()`)
is not related to this PR. I think a different `pbjson-build` version made it.
Please regenerate with the pinned version
(`./datafusion/proto-common/regen.sh`), so the diff only has the new field.
##########
datafusion/proto-models/src/from_proto.rs:
##########
@@ -355,6 +356,11 @@ impl TryFrom<&ParquetOptionsProto> for ParquetOptions {
"" => ParquetOptions::default().writer_version,
version => version.parse()?,
};
+ let row_group_range_assignment = match
proto.row_group_range_assignment.as_str() {
+ // Empty when encoded before this option existed
+ "" => RowGroupRangeAssignment::default(),
Review Comment:
Optional: this decode is the same as in
`proto-common/src/from_proto/mod.rs`. A small helper (for example a
`RowGroupRangeAssignment::from_proto_str`) would keep the two in sync.
##########
datafusion/datasource-parquet/src/row_group_filter.rs:
##########
@@ -46,19 +47,30 @@ pub struct RowGroupAccessPlanFilter {
access_plan: ParquetAccessPlan,
}
-/// Returns true if this row group belongs to `range`.
+/// Returns true if `assignment` assigns this row group to `range`.
///
-/// A row group belongs to the range containing its first dictionary/data page,
-/// so the ranges a file is split into for parallelism partition its row groups
-/// with none shared and none left over.
+/// Each row group maps to a single offset, so the ranges a file is split into
+/// for parallelism partition its row groups with none shared and none left
over.
///
/// Note: don't use the location of metadata
/// <https://github.com/apache/datafusion/issues/5995>
-pub(crate) fn row_group_in_range(metadata: &RowGroupMetaData, range:
&FileRange) -> bool {
+pub(crate) fn row_group_in_range(
+ metadata: &RowGroupMetaData,
+ range: &FileRange,
+ assignment: RowGroupRangeAssignment,
+) -> bool {
let col = metadata.column(0);
- let offset = col
- .dictionary_page_offset()
- .unwrap_or_else(|| col.data_page_offset());
+ let data_page_offset = col.data_page_offset();
+ let offset = match assignment {
+ RowGroupRangeAssignment::StartOffset => {
Review Comment:
Nit: this arm uses the dictionary offset even when it is after the first
data page, but the `Midpoint` arm takes the minimum. A short comment that says
this keeps the old behavior would help readers.
##########
datafusion/datasource-parquet/src/row_group_filter.rs:
##########
@@ -250,13 +262,27 @@ impl RowGroupAccessPlanFilter {
/// # Panics
/// if `groups.len() != self.len()`
pub fn prune_by_range(&mut self, groups: &[RowGroupMetaData], range:
&FileRange) {
Review Comment:
After this PR, the opener calls `prune_by_range_with_assignment`, and only
tests call `prune_by_range`. Can we keep one function? For example, add the
`assignment` argument to `prune_by_range`, or make the old one `#[cfg(test)]`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
