Minor: add with_estimated_selectivity to Precision
#8177
Conversation
| @@ -200,15 +200,10 @@ impl ExecutionPlan for FilterExec { | |||
| // assume filter selects 20% of rows if we cannot do anything smarter | |||
| // tracking issue for making this configurable: | |||
| // https://github.com/apache/arrow-datafusion/issues/8133 | |||
| let selectivity = 0.2_f32; | |||
| let selectivity = 0.2_f64; | |||
There was a problem hiding this comment.
Interestingly this code from @andygrove in #8126 is different to the way the selectivity is implemented below -- it uses f32 and doesn't apply ceil()
This PR makes sure these two paths are consistent which I think is an improvement
datafusion/common/src/stats.rs
Outdated
| /// Return the estimate of applying a filter of selectivity `selectivity` to | ||
| /// this Precision. A selectivity of `1.0` means that all rows are selected. | ||
| /// A selectivity of `0.5` means half the rows are selected. | ||
| pub fn apply_filter(self, selectivity: f64) -> Self { |
There was a problem hiding this comment.
This is one of the key APIs I expect to change as part of #8078 (the output will retain information about the min/max).
There was a problem hiding this comment.
I am wondering if it might be more suitable to locate this function in physical_plan's? There's already a multiplication function here, and it seems to me that this function could potentially be more relevant with filter context.
There was a problem hiding this comment.
This function both multiplies the values and turns the statistics into inexact (which I actually missed my first time through this code).
I think the intent is much clearer and consistent in this PR where the logic is commented in a function (rather than replicated 4 times, inconsistently).
That being said, I agree the notion of 'filter' is probably more specific to the physical plan rather than a "precision"
Perhaps we can come up with a different name for the function ? what about Precision::estimate_filtered or Precision::with_estimated_selectivity?
There was a problem hiding this comment.
I think Precision::with_estimated_selectivity sounds more descriptive.
| /// rows are selected. A selectivity of `0.5` means half the rows are | ||
| /// selected. Will always return inexact statistics. | ||
| pub fn apply_filter(self, selectivity: f64) -> Self { | ||
| self.map(|v| ((v as f64 * selectivity).ceil()) as usize) |
There was a problem hiding this comment.
Why use ceil in this case rather than round?
There was a problem hiding this comment.
Maybe Inexact(0) triggers some decisions at somewhere?
There was a problem hiding this comment.
Yeah, I think there were several cases where DataFusion was (incorrectly) optimizing away scans based on Inexact(0) -- I tried to capture some of what is going on in the #8227 ticket if you are interested
There was a problem hiding this comment.
Why use ceil in this case rather than round?
Also, specifically in this PR I used ceil() because that is what the existing code did in the older codepath.
alamb
changed the title
apply_filter to Precisionwith_estimated_selectivity to Precision
Which issue does this PR close?
Part of #8078
Rationale for this change
I am trying to consolidate uses of
Precisioninto methods on the Precision enum so thatWhat changes are included in this PR?
Filter::statisticsintoPrecision::apply_filterPrecision:: with_estimated_selectivityAre these changes tested?
Existing tests
Are there any user-facing changes?