Copilot commented on code in PR #22248:
URL: https://github.com/apache/datafusion/pull/22248#discussion_r3252883699
##########
datafusion/functions/src/math/ceil.rs:
##########
@@ -191,11 +191,66 @@ impl ScalarUDFImpl for CeilFunc {
}
fn evaluate_bounds(&self, inputs: &[&Interval]) -> Result<Interval> {
- let data_type = inputs[0].data_type();
- Interval::make_unbounded(&data_type)
+ let [input] = inputs else {
+ return Interval::make_unbounded(&DataType::Float64);
+ };
+ let data_type = input.data_type();
+ match (ceil_scalar(input.lower()), ceil_scalar(input.upper())) {
+ (Some(lo), Some(hi)) => Interval::try_new(lo, hi)
+ .or_else(|_| Interval::make_unbounded(&data_type)),
+ _ => Interval::make_unbounded(&data_type),
+ }
+ }
+
+ fn propagate_constraints(
+ &self,
+ interval: &Interval,
+ inputs: &[&Interval],
+ ) -> Result<Option<Vec<Interval>>> {
+ let [input_interval] = inputs else {
+ return Ok(Some(vec![]));
+ };
+ // ceil(x) ∈ [N, M] → x ∈ (N−1, M] — conservative closed: [N−1, M]
+ let lo = match interval.lower() {
+ ScalarValue::Float64(Some(n)) if n.is_finite() => {
+ Some(ScalarValue::Float64(Some(n - 1.0)))
+ }
+ ScalarValue::Float32(Some(n)) if n.is_finite() => {
+ Some(ScalarValue::Float32(Some(n - 1.0)))
+ }
+ _ => None,
+ };
+ let hi = match interval.upper() {
+ ScalarValue::Float64(Some(n)) if n.is_finite() => {
+ Some(ScalarValue::Float64(Some(*n)))
+ }
+ ScalarValue::Float32(Some(n)) if n.is_finite() => {
+ Some(ScalarValue::Float32(Some(*n)))
+ }
+ _ => None,
+ };
+ match (lo, hi) {
+ (Some(lo), Some(hi)) => {
+ let constraint = Interval::try_new(lo, hi)?;
+ Ok(input_interval.intersect(constraint)?.map(|r| vec![r]))
+ }
+ _ => Ok(Some(vec![])),
+ }
}
Review Comment:
Returning `Ok(Some(vec![]))` when arity is unexpected (and also when bounds
are non-finite) is ambiguous and can be unsafe if callers assume `Some(vec)`
has the same length as `inputs`. Prefer returning `Ok(None)` to indicate “no
propagation possible”, or return `Ok(Some(vec![input_interval.clone()]))` (or
equivalent) to explicitly represent “no change” while preserving the expected
vector length.
##########
datafusion/physical-expr/src/intervals/utils.rs:
##########
@@ -193,3 +200,109 @@ fn interval_dt_to_duration_ms(dt: &IntervalDayTime) ->
Result<i64> {
)
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::expressions::{Column, Literal};
+ use crate::scalar_function::ScalarFunctionExpr;
+ use arrow::datatypes::{Field, Schema};
+ use datafusion_common::ScalarValue;
+ use datafusion_common::config::ConfigOptions;
+
+ fn f64_schema() -> SchemaRef {
+ Arc::new(Schema::new(vec![Field::new("x", DataType::Float64, false)]))
+ }
+
+ fn utf8_schema() -> SchemaRef {
+ Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, false)]))
+ }
+
+ fn col_x() -> Arc<dyn PhysicalExpr> {
+ Arc::new(Column::new("x", 0))
+ }
+
+ fn lit_f64(v: f64) -> Arc<dyn PhysicalExpr> {
+ Arc::new(Literal::new(ScalarValue::Float64(Some(v))))
+ }
+
+ fn scalar_fn_expr(
+ udf: Arc<datafusion_expr::ScalarUDF>,
+ args: Vec<Arc<dyn PhysicalExpr>>,
+ return_type: DataType,
+ ) -> Arc<dyn PhysicalExpr> {
+ let name = udf.name().to_string();
+ Arc::new(ScalarFunctionExpr::new(
+ &name,
+ udf,
+ args,
+ Field::new("result", return_type, true).into(),
+ Arc::new(ConfigOptions::default()),
+ ))
+ }
+
+ #[test]
+ fn test_check_support_scalar_fn_supported_return_type() {
+ // ceil(x) returns Float64 — both return type and child are supported
+ let schema = f64_schema();
+ let expr = scalar_fn_expr(
+ datafusion_functions::math::ceil(),
+ vec![col_x()],
+ DataType::Float64,
+ );
+ assert!(check_support(&expr, &schema));
+ }
+
+ #[test]
+ fn test_check_support_scalar_fn_unsupported_return_type() {
+ // A UDF that returns Utf8 — not in is_datatype_supported
+ let schema = f64_schema();
+ let expr = scalar_fn_expr(
+ datafusion_functions::math::ceil(),
+ vec![col_x()],
+ DataType::Utf8,
+ );
+ assert!(!check_support(&expr, &schema));
+ }
Review Comment:
This test constructs a `ScalarFunctionExpr` using the `ceil` UDF but forces
the return type to `Utf8`, which is not a realistic/valid combination and may
break if `ScalarFunctionExpr::new` adds stricter validation. Consider creating
a small test-only UDF whose declared return type is `Utf8`, or using an
existing function that legitimately returns `Utf8`, so the test exercises
`check_support` on a valid expression tree.
##########
datafusion/functions/src/math/ceil.rs:
##########
@@ -191,11 +191,66 @@ impl ScalarUDFImpl for CeilFunc {
}
fn evaluate_bounds(&self, inputs: &[&Interval]) -> Result<Interval> {
- let data_type = inputs[0].data_type();
- Interval::make_unbounded(&data_type)
+ let [input] = inputs else {
+ return Interval::make_unbounded(&DataType::Float64);
Review Comment:
If `evaluate_bounds` is called with an unexpected number of inputs,
returning an unbounded `Float64` interval can produce a type mismatch (e.g.,
callers expecting `Float32`). Prefer returning an error for invalid arity, or
at least derive the unbounded interval’s data type from the available input(s)
when possible.
--
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]
