refactor & fix: refactor the interface of range filter selectivity, fix null frac (#155)

- Refactor the interface of `get_column_range_selectivity` so that
`BETWEEN` can also use this function.
- Fix NULL handling.
- Update unit tests accordingly.

Author: Gun9niR

optd-core/src/rel_node.rs

@@ -27,7 +27,7 @@ pub trait RelNodeTyp:
     fn list_typ() -> Self;
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub struct SerializableOrderedF64(pub OrderedFloat<f64>);
 
 impl Serialize for SerializableOrderedF64 {
@@ -51,7 +51,7 @@ impl<'de> Deserialize<'de> for SerializableOrderedF64 {
     }
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
 pub enum Value {
     UInt8(u8),
     UInt16(u16),

optd-datafusion-repr/src/cost/base_cost.rs

@@ -1,3 +1,4 @@
+use std::ops::Bound;
 use std::{collections::HashMap, sync::Arc};
 
 use crate::plan_nodes::{
@@ -985,34 +986,28 @@ impl<M: MostCommonValues, D: Distribution> OptCostModel<M, D> {
                             BinOpType::Neq => {
                                 self.get_column_equality_selectivity(table, *col_idx, value, false)
                             }
-                            BinOpType::Lt => self.get_column_range_selectivity(
-                                table,
-                                *col_idx,
-                                value,
-                                is_left_col_ref,
-                                false,
-                            ),
-                            BinOpType::Leq => self.get_column_range_selectivity(
-                                table,
-                                *col_idx,
-                                value,
-                                is_left_col_ref,
-                                true,
-                            ),
-                            BinOpType::Gt => self.get_column_range_selectivity(
-                                table,
-                                *col_idx,
-                                value,
-                                !is_left_col_ref,
-                                false,
-                            ),
-                            BinOpType::Geq => self.get_column_range_selectivity(
-                                table,
-                                *col_idx,
-                                value,
-                                !is_left_col_ref,
-                                true,
-                            ),
+                            BinOpType::Lt | BinOpType::Leq | BinOpType::Gt | BinOpType::Geq => {
+                                let start = match (comp_bin_op_typ, is_left_col_ref) {
+                                    (BinOpType::Lt, true) | (BinOpType::Geq, false) => Bound::Unbounded,
+                                    (BinOpType::Leq, true) | (BinOpType::Gt, false) => Bound::Unbounded,
+                                    (BinOpType::Gt, true) | (BinOpType::Leq, false) => Bound::Excluded(value),
+                                    (BinOpType::Geq, true) | (BinOpType::Lt, false) => Bound::Included(value),
+                                    _ => unreachable!("all comparison BinOpTypes were enumerated. this should be unreachable"),
+                                };
+                                let end = match (comp_bin_op_typ, is_left_col_ref) {
+                                    (BinOpType::Lt, true) | (BinOpType::Geq, false) => Bound::Excluded(value),
+                                    (BinOpType::Leq, true) | (BinOpType::Gt, false) => Bound::Included(value),
+                                    (BinOpType::Gt, true) | (BinOpType::Leq, false) => Bound::Unbounded,
+                                    (BinOpType::Geq, true) | (BinOpType::Lt, false) => Bound::Unbounded,
+                                    _ => unreachable!("all comparison BinOpTypes were enumerated. this should be unreachable"),
+                                };
+                                self.get_column_range_selectivity(
+                                    table,
+                                    *col_idx,
+                                    start,
+                                    end,
+                                )
+                            },
                             _ => unreachable!("all comparison BinOpTypes were enumerated. this should be unreachable"),
                         }
                     }
@@ -1148,56 +1143,61 @@ impl<M: MostCommonValues, D: Distribution> OptCostModel<M, D> {
         }
     }
 
-    /// Get the selectivity of an expression of the form "column </<=/>=/> value" (or "value </<=/>=/> column")
-    /// Computes selectivity based off of statistics
-    /// Range predicates are handled entirely differently from equality predicates so this is its own function
-    /// If it is unable to find the statistics, it returns None
-    /// Like in the Postgres source code, we decompose the four operators "</<=/>=/>" into "is_lt" and "is_eq"
-    /// The "is_lt" and "is_eq" values are set as if column is on the left hand side
-    fn get_column_range_selectivity(
+    /// Compute the frequency of values in a column less than or equal to the given value.
+    fn get_column_leq_value_freq(per_column_stats: &PerColumnStats<M, D>, value: &Value) -> f64 {
+        // because distr does not include the values in MCVs, we need to compute the CDFs there as well
+        // because nulls return false in any comparison, they are never included when computing range selectivity
+        let distr_leq_freq = per_column_stats.distr.cdf(value);
+        let value = value.clone();
+        let pred = Box::new(move |val: &Value| val <= &value);
+        let mcvs_leq_freq = per_column_stats.mcvs.freq_over_pred(pred);
+        distr_leq_freq + mcvs_leq_freq
+    }
+
+    /// Compute the frequency of values in a column less than the given value.
+    fn get_column_lt_value_freq(
         &self,
+        per_column_stats: &PerColumnStats<M, D>,
         table: &str,
         col_idx: usize,
         value: &Value,
-        is_col_lt_val: bool,
-        is_col_eq_val: bool,
+    ) -> f64 {
+        // depending on whether value is in mcvs or not, we use different logic to turn total_lt_cdf into total_leq_cdf
+        // this logic just so happens to be the exact same logic as get_column_equality_selectivity implements
+        Self::get_column_leq_value_freq(per_column_stats, value)
+            - self.get_column_equality_selectivity(table, col_idx, value, true)
+    }
+
+    /// Get the selectivity of an expression of the form "column </<=/>=/> value" (or "value </<=/>=/> column").
+    /// Computes selectivity based off of statistics.
+    /// Range predicates are handled entirely differently from equality predicates so this is its own function.
+    /// If it is unable to find the statistics, it returns DEFAULT_INEQ_SEL.
+    /// The selectivity is computed as quantile of the right bound minus quantile of the left bound.
+    fn get_column_range_selectivity(
+        &self,
+        table: &str,
+        col_idx: usize,
+        start: Bound<&Value>,
+        end: Bound<&Value>,
     ) -> f64 {
         if let Some(per_column_stats) = self.get_per_column_stats(table, col_idx) {
-            // because distr does not include the values in MCVs, we need to compute the CDFs there as well
-            // because nulls return false in any comparison, they are never included when computing range selectivity
-            let distr_leq_freq = per_column_stats.distr.cdf(value);
-            let value_clone = value.clone(); // clone the value so that we can move it into the closure to avoid lifetime issues
-                                             // TODO: in a future PR, figure out how to make Values comparable. rn I just hardcoded as_i32() to work around this
-            let pred = Box::new(move |val: &Value| val.as_i32() <= value_clone.as_i32());
-            let mcvs_leq_freq = per_column_stats.mcvs.freq_over_pred(pred);
-            let total_leq_freq = distr_leq_freq + mcvs_leq_freq;
-
-            // depending on whether value is in mcvs or not, we use different logic to turn total_leq_cdf into total_lt_cdf
-            // this logic just so happens to be the exact same logic as get_column_equality_selectivity implements
-            let total_lt_freq =
-                total_leq_freq - self.get_column_equality_selectivity(table, col_idx, value, true);
-
-            // use either total_leq_freq or total_lt_freq to get the selectivity
-            if is_col_lt_val {
-                if is_col_eq_val {
-                    // this branch means <=
-                    total_leq_freq
-                } else {
-                    // this branch means <
-                    total_lt_freq
+            let left_quantile = match start {
+                Bound::Unbounded => 0.0,
+                Bound::Included(value) => {
+                    self.get_column_lt_value_freq(per_column_stats, table, col_idx, value)
                 }
-            } else {
-                // clippy wants me to collapse this into an else if, but keeping two nested if else statements is clearer
-                #[allow(clippy::collapsible_else_if)]
-                if is_col_eq_val {
-                    // this branch means >=, which is 1 - < - null_frac
-                    // we need to subtract null_frac since that isn't included in >= either
-                    1.0 - total_lt_freq - per_column_stats.null_frac
-                } else {
-                    // this branch means >. same logic as above
-                    1.0 - total_leq_freq - per_column_stats.null_frac
+                Bound::Excluded(value) => Self::get_column_leq_value_freq(per_column_stats, value),
+            };
+            let right_quantile = match end {
+                Bound::Unbounded => 1.0,
+                Bound::Included(value) => Self::get_column_leq_value_freq(per_column_stats, value),
+                Bound::Excluded(value) => {
+                    self.get_column_lt_value_freq(per_column_stats, table, col_idx, value)
                 }
-            }
+            };
+            assert!(left_quantile <= right_quantile);
+            // `Distribution` does not account for NULL values, so the selectivity is smaller than frequency.
+            (right_quantile - left_quantile) * (1.0 - per_column_stats.null_frac)
         } else {
             DEFAULT_INEQ_SEL
         }
@@ -1541,7 +1541,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Leq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1565,18 +1565,18 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Leq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
         }];
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree, &column_refs),
-            0.7
+            0.7 * 0.9
         );
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree_rev, &column_refs),
-            0.7
+            0.7 * 0.9
         );
     }
 
@@ -1598,7 +1598,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Leq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1627,7 +1627,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Leq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1651,7 +1651,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Lt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1675,18 +1675,18 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Lt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
         }];
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree, &column_refs),
-            0.6
+            0.6 * 0.9
         );
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree_rev, &column_refs),
-            0.6
+            0.6 * 0.9
         );
     }
 
@@ -1708,7 +1708,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Lt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
         // TODO(phw2): make column_refs a function
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
@@ -1742,7 +1742,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Lt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Gt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Geq, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1768,7 +1768,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Gt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Lt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Leq, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1792,19 +1792,18 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Gt, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Lt, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Leq, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
         }];
-        // we have to subtract 0.1 since we don't want to include them in GT either
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree, &column_refs),
-            1.0 - 0.7 - 0.1
+            (1.0 - 0.7) * 0.9
         );
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree_rev, &column_refs),
-            1.0 - 0.7 - 0.1
+            (1.0 - 0.7) * 0.9
         );
     }
 
@@ -1818,7 +1817,7 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Geq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Leq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Lt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
@@ -1842,19 +1841,19 @@ mod tests {
             TestDistribution::new(vec![(Value::Int32(15), 0.7)]),
         ));
         let expr_tree = bin_op(BinOpType::Geq, col_ref(0), cnst(Value::Int32(15)));
-        let expr_tree_rev = bin_op(BinOpType::Leq, cnst(Value::Int32(15)), col_ref(0));
+        let expr_tree_rev = bin_op(BinOpType::Lt, cnst(Value::Int32(15)), col_ref(0));
         let column_refs = vec![ColumnRef::BaseTableColumnRef {
             table: String::from(TABLE1_NAME),
             col_idx: 0,
         }];
-        // we have to subtract 0.1 since we don't want to include them in GT either
+        // we have to add 0.1 since it's Geq
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree, &column_refs),
-            1.0 - 0.6 - 0.1
+            (1.0 - 0.7 + 0.1) * 0.9
         );
         assert_approx_eq::assert_approx_eq!(
             cost_model.get_filter_selectivity(expr_tree_rev, &column_refs),
-            1.0 - 0.6 - 0.1
+            (1.0 - 0.7 + 0.1) * 0.9
         );
     }