shrivasshankar commented on code in PR #27148:
URL: https://github.com/apache/datafusion/pull/27148#discussion_r4516401985
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -1249,6 +1256,56 @@ fn compute_fallback_selectivity(
selectivity
}
+/// Returns true if every decimal column in `predicate` has both a min and a
+/// max statistic.
+///
+/// Interval analysis cannot narrow a column without bounds, so it estimates a
+/// selectivity of 1.0 for it. Filters on aggregate outputs, such as
+/// `HAVING sum(l_quantity) > 300` in TPC-H Q18, usually have no bounds. These
+/// decimal predicates used the default selectivity before interval analysis
+/// supported decimals, and join planning depends on that estimate, so they
+/// keep using it.
+fn decimal_columns_have_bounds(
+ predicate: &Arc<dyn PhysicalExpr>,
+ schema: &SchemaRef,
+ column_statistics: &[ColumnStatistics],
+) -> bool {
+ collect_columns(predicate).iter().all(|column| {
+ let is_decimal = schema
+ .fields()
+ .get(column.index())
+ .is_some_and(|field| field.data_type().is_decimal());
+ !is_decimal
+ || column_statistics.get(column.index()).is_some_and(|stats| {
+ stats.min_value.get_value().is_some()
Review Comment:
covered by the cardinality change now. added your test as a case in
test_filter_statistics_without_bounds.
##########
datafusion/physical-expr/src/intervals/utils.rs:
##########
@@ -23,19 +23,57 @@ use datafusion_expr::interval_arithmetic::Interval;
/// will relax as more types of `PhysicalExpr`s and `Operator`s are supported.
/// Currently, [`CastExpr`], [`NegativeExpr`], [`BinaryExpr`], [`Column`] and
[`Literal`] are supported.
pub fn check_support(expr: &Arc<dyn PhysicalExpr>, schema: &SchemaRef) -> bool
{
+ check_support_with(expr, schema, is_datatype_supported)
+}
+
+/// Like [`check_support`], but additionally accepts decimal types. This is
+/// used when estimating filter statistics, where decimal predicates can be
+/// analyzed with interval arithmetic. Other users of [`check_support`], such
+/// as symmetric hash join pruning, do not support decimals yet.
+pub fn check_statistics_support(
+ expr: &Arc<dyn PhysicalExpr>,
+ schema: &SchemaRef,
+) -> bool {
+ check_support_with(expr, schema, |data_type| {
+ is_datatype_supported(data_type)
+ || matches!(
+ data_type,
+ DataType::Decimal32(_, _)
Review Comment:
done
--
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]
