You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/21 14:05:21 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #2048: Update to arrow/parquet 11.0
alamb opened a new pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048
Update datafusion to latest arrow and parquet release to unblock things like https://github.com/apache/arrow-datafusion/pull/1990 from @yjshen
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832248685
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
How about we change ReadOptions like:
```rust
pub struct ReadOptions {
predicates: Vec<Box<dyn Fn(&[RowGroupMetaData]) -> vec<bool>>>,
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r831228639
##########
File path: datafusion/tests/parquet_pruning.rs
##########
@@ -262,7 +262,7 @@ async fn prune_int32_scalar_fun() {
println!("{}", output.description());
// This should prune out groups with error, because there is not col to
// prune the row groups.
- assert_eq!(output.predicate_evaluation_errors(), Some(1));
+ assert_eq!(output.predicate_evaluation_errors(), Some(4));
Review comment:
We are evaluating the filter for each row group now. I think it's an expected change for evaluation errors.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#issuecomment-1073949160
Yes, I've introduced a parquet row group filtering API change in parquet 11. I can port that part from #1990 to your branch.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832334400
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
That would definitely be one option, but I'm not sure why it needs to be lazy. `SerializedFileReader` already exposes the `ParquetMetadata` which in turn contains the `[RowGroupMetaData]. Why wouldn't the caller just specify the row groups to scan, much like it specifies the column indexes for a projection?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r831518031
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -383,17 +375,14 @@ macro_rules! get_null_count_values {
return None;
};
- let scalar_values: Vec<ScalarValue> = $self
- .row_group_metadata
- .iter()
- .flat_map(|meta| meta.column(column_index).statistics())
- .map(|stats| {
- ScalarValue::UInt64(Some(stats.null_count().try_into().unwrap()))
- })
- .collect();
-
- // ignore errors converting to arrays (e.g. different types)
- ScalarValue::iter_to_array(scalar_values).ok()
+ let value = ScalarValue::UInt64(
+ $self
+ .row_group_metadata
+ .column(column_index)
+ .statistics()
Review comment:
:+1:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832248685
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
How about we change [ReadOptions](https://github.com/apache/arrow-rs/blob/master/parquet/src/file/serialized_reader.rs#L177-L179) like:
```rust
pub struct ReadOptions {
predicates: Vec<Box<dyn Fn(&[RowGroupMetaData]) -> vec<bool>>>,
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832334400
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
That would definitely be one option, but I'm not sure why it needs to be lazy. `SerializedFileReader` already exposes the `ParquetMetadata` which in turn exposes the `[RowGroupMetaData]`. Why wouldn't the caller just specify the row groups to scan, much like it specifies the column indexes for a projection? Would this not be both simpler and more flexible?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r831415197
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
there is probably some overhead here related to calling `prune` once per row group vs calling it once per file, but I think it will be ok and we can further optimize it in the future if it shows up in traces.
##########
File path: datafusion/tests/parquet_pruning.rs
##########
@@ -262,7 +262,7 @@ async fn prune_int32_scalar_fun() {
println!("{}", output.description());
// This should prune out groups with error, because there is not col to
// prune the row groups.
- assert_eq!(output.predicate_evaluation_errors(), Some(1));
+ assert_eq!(output.predicate_evaluation_errors(), Some(4));
Review comment:
yeah, I agree.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#issuecomment-1073944895
I think this will need some of the changes in https://github.com/apache/arrow-datafusion/pull/1990 to `datafusion/src/physical_plan/file_format/parquet.rs` order to compile. I'll try and get around to it later this week if no one else beats me to it
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832152884
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
Yeah... I just stumbled across this whilst updating #1617 - in IOx we found the prune method had non-trivial overheads when run in a non-columnar fashion as this is doing. Admittedly that was likely with more containers than there are likely to be row groups in a file.
I do wonder if we need to take a step back from extending the parquet arrow-rs interface, and take a more holistic look at what the desired end-state should be. I worry a bit that we're painting ourselves into a corner, I'll see if I can get my thoughts into some tickets
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r831228639
##########
File path: datafusion/tests/parquet_pruning.rs
##########
@@ -262,7 +262,7 @@ async fn prune_int32_scalar_fun() {
println!("{}", output.description());
// This should prune out groups with error, because there is not col to
// prune the row groups.
- assert_eq!(output.predicate_evaluation_errors(), Some(1));
+ assert_eq!(output.predicate_evaluation_errors(), Some(4));
Review comment:
We are evaluating the filter for each row group now. I think it's an expected change for the number of evaluation errors.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a change in pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048#discussion_r832334400
##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -418,31 +407,33 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
fn build_row_group_predicate(
pruning_predicate: &PruningPredicate,
metrics: ParquetFileMetrics,
- row_group_metadata: &[RowGroupMetaData],
-) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
- let parquet_schema = pruning_predicate.schema().as_ref();
-
- let pruning_stats = RowGroupPruningStatistics {
- row_group_metadata,
- parquet_schema,
- };
- let predicate_values = pruning_predicate.prune(&pruning_stats);
-
- match predicate_values {
- Ok(values) => {
- // NB: false means don't scan row group
- let num_pruned = values.iter().filter(|&v| !*v).count();
- metrics.row_groups_pruned.add(num_pruned);
- Box::new(move |_, i| values[i])
- }
- // stats filter array could not be built
- // return a closure which will not filter out any row groups
- Err(e) => {
- debug!("Error evaluating row group predicate values {}", e);
- metrics.predicate_evaluation_errors.add(1);
- Box::new(|_r, _i| true)
- }
- }
+) -> Box<dyn FnMut(&RowGroupMetaData, usize) -> bool> {
+ let pruning_predicate = pruning_predicate.clone();
+ Box::new(
+ move |row_group_metadata: &RowGroupMetaData, _i: usize| -> bool {
+ let parquet_schema = pruning_predicate.schema().as_ref();
+ let pruning_stats = RowGroupPruningStatistics {
+ row_group_metadata,
+ parquet_schema,
+ };
+ let predicate_values = pruning_predicate.prune(&pruning_stats);
Review comment:
That would definitely be one option, but I'm not sure why it needs to be lazy. `SerializedFileReader` already exposes the `ParquetMetadata` which in turn exposes the `[RowGroupMetaData]`. Why wouldn't the caller just specify the row groups to scan, much like it specifies the column indexes for a projection?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #2048: Update to arrow/parquet 11.0
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #2048:
URL: https://github.com/apache/arrow-datafusion/pull/2048
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org