kosiew commented on code in PR #25790:
URL: https://github.com/apache/datafusion/pull/25790#discussion_r4334343675
##########
datafusion/physical-expr/src/intervals/utils.rs:
##########
@@ -36,19 +36,47 @@ 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!(
Review Comment:
Decimal arithmetic can propagate bounds with the wrong source type. For `a:
Decimal128(15,2)` with statistics `[1.00,10.00]`, `a * Decimal128(2.00,15,2) >
Decimal128(5.0000,31,4)` makes `FilterExec::try_new` fail with `Selectivity is
out of limit: 8324028856.826859` because inverse propagation retains scale 4;
`a - 2.00 > 5.00` also publishes Decimal128(17,2) bounds for a Decimal128(15,2)
column. Please restore source-typed bounds with conservative outward rounding,
or exclude decimal arithmetic from statistics eligibility. Clamping selectivity
alone is insufficient. Add regressions for multiplication
construction/selectivity and subtraction min/max types.
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -953,6 +980,44 @@ mod tests {
Ok(())
}
+ #[test]
Review Comment:
Optional follow-up: consider representative containment tests for negative
scales, cross-width widening, and bounds near precision limits. The exhaustive
test covers only Decimal128 scale 2 to 3 near zero, while the Decimal32 to
Decimal128 case currently checks eligibility rather than boundary propagation.
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -2050,6 +2050,124 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn test_filter_statistics_decimal_expr() -> Result<()> {
+ // Table:
+ // a: Decimal128(15, 2), min=0.00, max=0.10
+ let decimal = |v: i128| ScalarValue::Decimal128(Some(v), 15, 2);
+ let schema =
+ Schema::new(vec![Field::new("a", DataType::Decimal128(15, 2),
false)]);
+ let input = Arc::new(StatisticsExec::new(
+ Statistics {
+ num_rows: Precision::Inexact(1100),
+ total_byte_size: Precision::Absent,
+ column_statistics: vec![ColumnStatistics {
+ min_value: Precision::Inexact(decimal(0)),
+ max_value: Precision::Inexact(decimal(10)),
+ ..Default::default()
+ }],
+ },
+ schema.clone(),
+ ));
+
+ // a >= 0.05 AND a < 0.08
+ let predicate: Arc<dyn PhysicalExpr> = binary(
+ binary(col("a", &schema)?, Operator::GtEq, lit(decimal(5)),
&schema)?,
+ Operator::And,
+ binary(col("a", &schema)?, Operator::Lt, lit(decimal(8)),
&schema)?,
+ &schema,
+ )?;
+
+ // WHERE a >= 0.05 AND a < 0.08
+ let filter: Arc<dyn ExecutionPlan> =
+ Arc::new(FilterExec::try_new(predicate, input)?);
+
+ // The strict bound tightens to 0.07, so [0.05, 0.07] keeps 3 of the 11
+ // representable values in [0.00, 0.10].
+ let statistics =
+ StatisticsContext::new().compute(filter.as_ref(),
&StatisticsArgs::new())?;
+ assert_eq!(statistics.num_rows, Precision::Inexact(300));
+ assert_eq!(
+ statistics.column_statistics,
+ vec![ColumnStatistics {
+ null_count: Precision::Exact(0),
+ min_value: Precision::Inexact(decimal(5)),
+ max_value: Precision::Inexact(decimal(7)),
+ ..Default::default()
+ }]
+ );
+
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn test_filter_statistics_decimal_mixed_scale_expr() -> Result<()> {
+ // Table:
+ // a: Decimal128(15, 2), min=0.00, max=0.10
+ let schema =
+ Schema::new(vec![Field::new("a", DataType::Decimal128(15, 2),
false)]);
+ let input = Arc::new(StatisticsExec::new(
+ Statistics {
Review Comment:
Optional follow-up: consider a SQLLogicTest for a Q6-style decimal range
with ordinary fractional literals and known min/max statistics, asserting
EXPLAIN estimates and query results. The current mixed-scale test constructs
the cast explicitly, so it does not cover SQL coercion through physical
expression construction to statistics estimation.
--
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]
