asolimando commented on issue #25616: URL: https://github.com/apache/datafusion/issues/25616#issuecomment-6366672963
Thanks @rgbuilds for the ping and for looking into this. Quick meta point first: I understand the topic is complex and there is a lot to discuss, but the message was too long and I had to use an LLM to go through it in reasonable time, I’d appreciate if you did that summarization on your end next time. Missing residual filter: `estimate_join_statistics` doesn’t take the residual filter, that’s a known limitation stated in the doc, I think we can improve it, especially since adding that param is not a breaking change. On the estimator: I agree that multiplying the ranges from interval analyzer is not what we want, that only works for single-column predicates (like `a < 10`), we need a pair-probability estimator, and this will need to be called wherever we can reference predicates (`WHERE`, `JOIN` etc.). I’d put it in `expr-common/src/interval_arithmetic.rs` as a pure function, something like `comparison_selectivity(op, left: &Interval, right: &Interval)`, with no join concepts in the signature: that's the piece that is the same no matter where it is called from, and `PhysicalExpr` will reference it from the comparison branch when the expression-level support comes. But if you want to support the repro in this issue, you also need to handle the cast, which is exactly the “navigation” the expression-level framework would give you for free, if you wait for it. Again, you can put that in a pure function somewhere, but you see the pattern, we are creating a lot of ad-hoc pieces to compensate for a missing framework, that’s why I am OK adding some support now (`comparison_selectivity` in this case, as that will be re-used as-is), but supporting more complex cases needs a lot of special-casing that will later go away, for that it’s worth waiting for the expression-level framework. Side note: what you propose with min/max is good, the expression-level support with extended statistics will allow even more precise estimations, if you have quantile sketches for the involved columns, for instance. (you can peek into the branch mentioned [here](https://github.com/apache/datafusion/pull/21122#issuecomment-5970893039) to see how sketches and advanced custom stats could be used). In summary, for a first PR I’d keep it simple with what we won’t need to remove later: - the `comparison_selectivity` helper on intervals, - the filter parameter addition, - column mapping to the filter’s intermediate schema stats, - apply the selectivity for inner joins (column vs column only, no cast handling), - fall back to the current estimate otherwise, - inner-join only, the rest needs a more complex treatment that needs a separate PR On the repro itself you’ll still hit the fallback because of the cast, that’s expected and it will only partially close this issue. Add a test on a cast-free column-vs-column join asserting the pair probability (0.49 on [1, 50] vs [1, 50]), and one on the repro documenting the gap, that one will improve once the expression-level estimator is in. Does this make sense to you? -- 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]
