mohitgurav20 commented on issue #25778: URL: https://github.com/apache/datafusion/issues/25778#issuecomment-5852643378
Really glad you wrote this up — I've been staring at the Q72 regression in optional_filter_min_saving_ns_per_row for a while and the prototype numbers make the problem crystal clear. The gap between the 1.3 ns real saving on Q10 vs. the 20 ns estimate is exactly why a constant will always be a losing game. I dug into the FilterExec stream and metrics code after reading this and had a few thoughts — dropping them here in case they're useful for the design questions. The confounding-filter problem (failure modes 3–5) All three of those failure modes share the same root: the off/on sample is polluted by other filters on the same scan changing state mid-measurement. One approach I kept coming back to is a monotone filter-state generation counter per scan site — a simple u64 that increments any time any filter on that site toggles. You only accept a timing sample if the generation number didn't change between when the batch was issued and when it was returned. It's cheap to check, requires no bitmask diffing, and would naturally discard the DS Q10 case (date filter active during ss_customer_sk sampling) and the DS Q79 case without needing continuous re-sampling. If you want finer-grained isolation (e.g. attributing exactly which filter changed), a compact active-filter bitmask per batch works too — but the generation counter feels like the right starting point since it's a single atomic compare. The "probe is not free" cases (Q72, Q54) For Q54 specifically, the probe isn't just not free — it enables 32k rows on the build side that otherwise wouldn't exist. I noticed probe_hit_rate is already tracked in joins/utils.rs (JoinMetrics.probe_hit_rate). That's an observable signal: if build_rows_added_by_probe > 0, the probe is not free by definition and should never be treated as amortizable. Wiring that into the pausing decision seems tractable without new constants. Work past an exchange The elapsed_compute timer in FilterExecMetrics already stops at a RepartitionExec boundary — so any downstream saving past an exchange is invisible to the measurement. I wonder if we could propagate a "time credit" upward through the post-execution plan tree in a way similar to how statistics_from_inputs walks children today. The exchange node would expose its downstream elapsed time, the filter walks up to claim it. It avoids instrumenting every intermediate operator and fits naturally into the existing metrics architecture. Quick scoping question In the prototype branch — is the measurement hooked into FilterExec::poll_next or is it closer to the scan/datasource level? If it's at the filter, the off→on delta naturally includes the filter eval overhead which is what we want. If it's at the scan, we'd need to be careful not to double-count predicate evaluation time. I'd genuinely like to help move one of the open design questions forward if you're open to it. The generation-counter approach for the confounding-filter failure modes feels like something that could be prototyped pretty quickly — happy to take a first pass at it if that's useful. -- 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]
