You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2025/10/22 20:46:31 UTC
[PR] Push partition_statistics into DataSource [datafusion]
adriangb opened a new pull request, #18233:
URL: https://github.com/apache/datafusion/pull/18233
Removes a downcast match in favor of use of the trait. This mirrors the changes to DataSourceExec to use partition_statistics instead of statistics.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on code in PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#discussion_r2458848721
##########
datafusion/datasource/src/source.rs:
##########
@@ -151,7 +151,7 @@ pub trait DataSource: Send + Sync + Debug {
fn scheduling_type(&self) -> SchedulingType {
SchedulingType::NonCooperative
}
- fn statistics(&self) -> Result<Statistics>;
+ fn partition_statistics(&self, partition: Option<usize>) -> Result<Statistics>;
Review Comment:
This is an API changing, I think we should make `statistics` as deprecated, and add a new partition_statistics API
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#discussion_r2453352022
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -2512,4 +2543,91 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_partition_statistics_projection() {
Review Comment:
This test demonstrates the bug and fails on `main`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#discussion_r2453404767
##########
datafusion/datasource/src/memory.rs:
##########
@@ -192,12 +193,27 @@ impl DataSource for MemorySourceConfig {
SchedulingType::Cooperative
}
- fn statistics(&self) -> Result<Statistics> {
- Ok(common::compute_record_batch_statistics(
- &self.partitions,
- &self.schema,
- self.projection.clone(),
- ))
+ fn partition_statistics(&self, partition: Option<usize>) -> Result<Statistics> {
+ if let Some(partition) = partition {
+ // Compute statistics for a specific partition
+ if let Some(batches) = self.partitions.get(partition) {
+ Ok(common::compute_record_batch_statistics(
+ from_ref(&batches),
+ &self.schema,
+ self.projection.clone(),
+ ))
+ } else {
+ // Invalid partition index
+ Ok(Statistics::new_unknown(&self.projected_schema))
+ }
+ } else {
+ // Compute statistics across all partitions
+ Ok(common::compute_record_batch_statistics(
+ &self.partitions,
+ &self.schema,
+ self.projection.clone(),
+ ))
+ }
}
Review Comment:
Previously unimplemented because only FileScanConfig was handled via downcast matching.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#discussion_r2453320357
##########
datafusion/datasource/src/source.rs:
##########
@@ -285,21 +285,7 @@ impl ExecutionPlan for DataSourceExec {
}
fn partition_statistics(&self, partition: Option<usize>) -> Result<Statistics> {
- if let Some(partition) = partition {
- let mut statistics = Statistics::new_unknown(&self.schema());
- if let Some(file_config) =
- self.data_source.as_any().downcast_ref::<FileScanConfig>()
- {
- if let Some(file_group) = file_config.file_groups.get(partition) {
- if let Some(stat) = file_group.file_statistics(None) {
- statistics = stat.clone();
- }
- }
- }
- Ok(statistics)
- } else {
- Ok(self.data_source.statistics()?)
- }
Review Comment:
This was clearly buggy: in the `partition = None` path it returns *projected* statistics (see current implementation in `FileScanConfig::statistics`), if it took the `Some(partition)` path it calculated the statistics but then never projected them.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#discussion_r2463021017
##########
datafusion/datasource/src/source.rs:
##########
@@ -151,7 +151,7 @@ pub trait DataSource: Send + Sync + Debug {
fn scheduling_type(&self) -> SchedulingType {
SchedulingType::NonCooperative
}
- fn statistics(&self) -> Result<Statistics>;
+ fn partition_statistics(&self, partition: Option<usize>) -> Result<Statistics>;
Review Comment:
Good call. I added `fn statistics()` back to `DataSource` trait as a deprecated method and made it default to `partition_statistics(None)`. This should make it fully backwards compatible for all implementations.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #18233:
URL: https://github.com/apache/datafusion/pull/18233#issuecomment-3449044223
Thanks for the review @xudong963 !
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Push partition_statistics into DataSource [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #18233:
URL: https://github.com/apache/datafusion/pull/18233
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org