comphead commented on code in PR #25940:
URL: https://github.com/apache/datafusion/pull/25940#discussion_r4169958501
##########
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:
Done in e2d43c38b, the diff now only adds the new field. FYI the pinned
`pbjson-build = "=0.9.0"` that `regen.sh` uses reproduces those three hunks,
because main's copy has hand edits the generator doesn't make (for example
`.copied()` from #24566). So I restored main's version and kept only the field
hunks.
##########
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:
Done in e2d43c38b. `prune_by_range` now takes the `RowGroupRangeAssignment`
argument, `prune_by_range_with_assignment` is gone, and the 56.0.0 upgrade
guide notes the change.
##########
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:
Done in e2d43c38b, added a comment that the `StartOffset` arm keeps the old
rule.
##########
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:
Done in e2d43c38b. Both decoders now call
`RowGroupRangeAssignment::from_proto_str`.
--
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]
