adriangb commented on PR #23420: URL: https://github.com/apache/datafusion/pull/23420#issuecomment-4958698430
> This doesn't resolve the whole TPCH regression from the earlier comment. Remaining gap between https://github.com/apache/datafusion/pull/23532 (644 ms) and pushdown=false baseline (531 ms) is the fundamental two-phase-read tax on queries where selectivity + narrow non-filter projection don't repay the coordination overhead (Q12 is the archetype). That looks like it needs the full adaptive-filter direction — @adriangb's stack (https://github.com/apache/datafusion/pull/22384 → https://github.com/apache/datafusion/pull/22237) is the right shape but is stalled; happy to help push https://github.com/apache/datafusion/pull/22384 forward first since it's the foundation. The fundamental issue is that (1) evaluating partition routing hashes is expensive and (2) is sometimes a big win if the join is selective but is the join is a 90% match you end up just paying the hashing price twice for no benefit, especially if the late materialization benefit is small as well. We can't know this statically, so yes I think the only way to work around it is to dynamically drop dynamic filters that are not earning their keep. > happy to help push https://github.com/apache/datafusion/pull/22384 forward first since it's the foundation I think that would be very nice. It is both a good chunk of the code but also resolves some theoretical issues that mean we could return wrong results in cases where `ParquetSource` accepts a filter for pushdown but then based on the physical schema cannot actually evaluate it (I haven't been able to find a concrete case where this happens, but it's a latent foot gun). I think the main blocker there is the benchmark numbers, in particular hitting the same issue as above: currently if filter pushdown is off the hash join dynamic filters bubble back up to the join in most cases and then don't get evaluated anywhere. If we keep them and evaluate them in parquet scan it means they will always get evaluated so any cases where they were not earning their keep will show up as performance regressions. I do wonder if one path forward isn't to attack that specific case by e.g. splitting out the `min/max` range dynamic filters that `HashJoinExec` pushes down from the hash table ones and then we could turn off the hash table ones by default (a minor breaking change but I think justifiable if benchmarks improve or it unblocks other work). -- 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]
