asolimando commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3630328837
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -182,35 +230,245 @@ impl StatisticsContext {
requests.len(),
children.len()
);
- let child_stats = children
+ let child_statistics: Vec<Arc<Statistics>> = children
Review Comment:
Thanks @xudong963 for your review.
This topic actually emerged during my local AI-assisted reviews, but I went
with the current approach for two reasons:
1. I considered the `CustomExec` change as part of its migration towards the
new `child_stats_requests` format, as it was pre-dating it
2. I couldn’t come up with a strong example motivating that a provider might
really need different child statistics than what the operator suggests, as all
I could think were cases where the nature of the operator drives it completely
(in a join the build side usually needs the overall statistics, per-partition
on the probe side)
But you are right reminding the `StatisticRegistry`’s contract, so for 1.
even if a migration is probably needed, the framework should still work before
that happens. And `UnnestExec` is the good example for 2. I couldn’t come up
with.
In e4df6301d27f7aaa89add5470760c69b39f723d5, I have recovered the full
original capability of `StatisticsRegistry::compute` by giving
`StatisticsProvider` its own `child_stats_requests` (defaulted to request every
child), resolved per provider in the walk, independently of the operator's. A
provider can now refine a `Skip` operator without touching it.
Resolution is memoized and keyed by `(node, partition)`, so a provider's
request and the operator's coexist as separate cache entries. The no-provider
path is unchanged with no extra overhead.
As a proof, I have reverted the “migration” of `CustomExec` (removed the
override of `child_stats_requests`) and the test still passes.
--
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]
