You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/03 00:50:21 UTC
(datafusion) branch gh-readonly-queue/main/pr-25967-c8a4ce37247e9c8495c77661a959f3fea4089116 created (now cae7c614c7)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-25967-c8a4ce37247e9c8495c77661a959f3fea4089116
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at cae7c614c7 test: strengthen scalar UDF type recovery regression coverage (#25967)
This branch includes the following new commits:
new cae7c614c7 test: strengthen scalar UDF type recovery regression coverage (#25967)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: test: strengthen scalar UDF type recovery regression coverage (#25967)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25967-c8a4ce37247e9c8495c77661a959f3fea4089116
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit cae7c614c768464d2bf82f28fc4afc9401219ab8
Author: Malingshu <69...@users.noreply.github.com>
AuthorDate: Sat Oct 3 00:49:57 2026 +0000
test: strengthen scalar UDF type recovery regression coverage (#25967)
## Which issue does this PR close?
- Closes #25925.
## Rationale for this change
The time arithmetic SQL tests added in #25668 retain their sorts even
when scalar UDF type recovery is removed, because ordinary arithmetic
already treats unbounded results as potentially overflowing. Checked
arithmetic needs the recovered time type to detect wrapping across
midnight, so it provides a regression test for that interaction.
## What changes are included in this PR?
- Add a checked `date_trunc('hour', t) + INTERVAL '2 hours'` property
test over ordered Time64 input. Assert that truncation preserves
ordering before checking that time arithmetic drops it.
- Cover an override returning unbounded Null bounds, and separate
bounds-preservation tests from error propagation.
- Clarify which intervals are replaced or preserved and that direct
`PhysicalExpr::evaluate_bounds` calls bypass the fallback.
## What is the testing strategy for this PR?
The new checked arithmetic test passes with type recovery and fails with
`Ordered` instead of `Unordered` when the fallback is removed. Existing
SQL tests are retained.
Validation: `cargo clippy --all-targets --all-features -- -D warnings`,
`./dev/rust_lint.sh`, the extended workspace test suite, and all 8
scalar-function unit tests. Ablation checks confirm that removing type
recovery fails the ordering assertion, and making the truncated child
unordered fails the new precondition assertion.
## Are there any user-facing changes?
UDF bounds documentation is more precise. Execution behavior and public
APIs are unchanged.
---
datafusion/expr/src/udf.rs | 17 ++-
datafusion/physical-expr/src/scalar_function.rs | 165 +++++++++++++++++-------
2 files changed, 125 insertions(+), 57 deletions(-)
diff --git a/datafusion/expr/src/udf.rs b/datafusion/expr/src/udf.rs
index fd2e7b96be..d937a54029 100644
--- a/datafusion/expr/src/udf.rs
+++ b/datafusion/expr/src/udf.rs
@@ -910,13 +910,16 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any {
/// The default implementation returns an unbounded [`DataType::Null`]
/// interval because the output type cannot be inferred from input intervals
/// alone. During physical property analysis, `ScalarFunctionExpr` uses its
- /// resolved return type to replace this default with a typed unbounded
- /// interval when supported. UDFs need not override this method solely to
- /// preserve their output type for that analysis.
- ///
- /// Override this method to provide more precise bounds. Explicit bounds
- /// are preserved. The fallback does not apply to direct bounds evaluation
- /// by the constraint solver.
+ /// resolved return type to replace any unbounded [`DataType::Null`]
+ /// interval returned here with a typed unbounded interval when supported,
+ /// including intervals returned by overrides. If the resolved type is not
+ /// supported by [`Interval::make_unbounded`], the original interval is kept.
+ /// UDFs need not override this method solely to preserve their output type
+ /// for that analysis.
+ ///
+ /// Override this method to provide more precise bounds. All intervals other
+ /// than an unbounded [`DataType::Null`] interval are preserved. The fallback
+ /// does not apply to direct calls to `PhysicalExpr::evaluate_bounds`.
fn evaluate_bounds(&self, _input: &[&Interval]) -> Result<Interval> {
// We cannot assume the input datatype is the same of output type.
Interval::make_unbounded(&DataType::Null)
diff --git a/datafusion/physical-expr/src/scalar_function.rs b/datafusion/physical-expr/src/scalar_function.rs
index 2245f63eef..a2e6c73eb4 100644
--- a/datafusion/physical-expr/src/scalar_function.rs
+++ b/datafusion/physical-expr/src/scalar_function.rs
@@ -322,8 +322,8 @@ impl PhysicalExpr for ScalarFunctionExpr {
.map(|props| &props.range)
.collect::<Vec<_>>();
let range = self.fun().evaluate_bounds(&children_range)?;
- // The default UDF bounds carry no type information. Recover the resolved
- // output type for property inference without discarding explicit bounds.
+ // Unbounded Null bounds carry no type information, even from an override.
+ // Recover the resolved output type while preserving all other intervals.
let range = if range.data_type() == DataType::Null && range.is_unbounded() {
Interval::make_unbounded(self.return_type()).unwrap_or(range)
} else {
@@ -433,7 +433,7 @@ mod tests {
assert_eq!(properties.sort_properties, SortProperties::Unordered);
assert!(!properties.preserves_lex_ordering);
assert!(!properties.strictly_order_preserving);
- // The constraint solver calls this method directly, bypassing the fallback.
+ // Direct bounds evaluation does not use the property-analysis fallback.
assert_eq!(
expr.evaluate_bounds(&[]).unwrap(),
Interval::make_unbounded(&DataType::Null).unwrap()
@@ -527,6 +527,55 @@ mod tests {
}
}
+ #[test]
+ fn checked_time_arithmetic_uses_recovered_function_type() -> Result<()> {
+ use crate::expressions::BinaryExpr;
+ use arrow::compute::SortOptions;
+ use datafusion_expr::Operator;
+ use datafusion_functions::datetime::date_trunc::DateTruncFunc;
+
+ let time_type = DataType::Time64(TimeUnit::Nanosecond);
+ let precision = Arc::new(Literal::new(ScalarValue::from("hour")));
+ let precision_properties = precision.get_properties(&[])?;
+ let date_trunc = ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(DateTruncFunc::new())),
+ vec![precision, Arc::new(Column::new("t", 0))],
+ &Schema::new(vec![Field::new("t", time_type.clone(), true)]),
+ Arc::new(ConfigOptions::default()),
+ )?;
+ let time_properties = ExprProperties::new_unknown()
+ .with_order(SortProperties::Ordered(SortOptions::default()))
+ .with_range(Interval::make_unbounded(&time_type)?);
+ let truncated_properties =
+ date_trunc.get_properties(&[precision_properties, time_properties])?;
+ assert_eq!(
+ truncated_properties.range,
+ Interval::make_unbounded(&time_type)?
+ );
+ assert_eq!(
+ truncated_properties.sort_properties,
+ SortProperties::Ordered(SortOptions::default())
+ );
+ let interval = Arc::new(Literal::new(ScalarValue::new_interval_mdn(
+ 0,
+ 0,
+ 7_200_000_000_000,
+ )));
+ let interval_properties = interval.get_properties(&[])?;
+ let shifted = BinaryExpr::new(Arc::new(date_trunc), Operator::Plus, interval)
+ .with_fail_on_overflow(true);
+
+ // Checked arithmetic still wraps time-of-day across midnight. Recovering
+ // the function's type is necessary to recognize this non-monotonic case.
+ assert_eq!(
+ shifted
+ .get_properties(&[truncated_properties, interval_properties])?
+ .sort_properties,
+ SortProperties::Unordered
+ );
+ Ok(())
+ }
+
#[derive(Debug, PartialEq, Eq, Hash)]
struct BoundsUDF {
inner: MockScalarUDF,
@@ -558,57 +607,73 @@ mod tests {
}
}
+ fn bounds_expr(fail: bool) -> ScalarFunctionExpr {
+ ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(BoundsUDF {
+ inner: MockScalarUDF {
+ signature: Signature::exact(
+ vec![DataType::Int32],
+ Volatility::Immutable,
+ ),
+ return_type: DataType::Int32,
+ },
+ fail,
+ })),
+ vec![Arc::new(Column::new("a", 0))],
+ &Schema::new(vec![Field::new("a", DataType::Int32, true)]),
+ Arc::new(ConfigOptions::default()),
+ )
+ .unwrap()
+ }
+
#[test]
- fn properties_preserve_udf_bounds_and_errors() {
- for fail in [false, true] {
- let expr = ScalarFunctionExpr::try_new(
- Arc::new(ScalarUDF::from(BoundsUDF {
- inner: MockScalarUDF {
- signature: Signature::exact(
- vec![DataType::Int32],
- Volatility::Immutable,
- ),
- return_type: DataType::Int32,
- },
- fail,
- })),
- vec![Arc::new(Column::new("a", 0))],
- &Schema::new(vec![Field::new("a", DataType::Int32, true)]),
- Arc::new(ConfigOptions::default()),
- )
- .unwrap();
- for (lower, upper) in [
- (Some(0), Some(100)),
- (Some(1), Some(1)),
- (None, Some(100)),
- (Some(0), None),
- (None, None),
- ] {
- let bounds = Interval::make::<i32>(lower, upper).unwrap();
- let child = ExprProperties::new_unknown().with_range(bounds.clone());
- let properties = expr.get_properties(&[child]);
- let direct = expr.evaluate_bounds(&[&bounds]);
- if fail {
- assert!(
- properties
- .unwrap_err()
- .to_string()
- .contains("bounds evaluation failed")
- );
- assert!(
- direct
- .unwrap_err()
- .to_string()
- .contains("bounds evaluation failed")
- );
- } else {
- assert_eq!(properties.unwrap().range, bounds);
- assert_eq!(direct.unwrap(), bounds);
- }
- }
+ fn properties_preserve_udf_bounds() {
+ let expr = bounds_expr(false);
+ for (lower, upper) in [
+ (Some(0), Some(100)),
+ (Some(1), Some(1)),
+ (None, Some(100)),
+ (Some(0), None),
+ (None, None),
+ ] {
+ let bounds = Interval::make::<i32>(lower, upper).unwrap();
+ let child = ExprProperties::new_unknown().with_range(bounds.clone());
+ assert_eq!(expr.get_properties(&[child]).unwrap().range, bounds);
+ assert_eq!(expr.evaluate_bounds(&[&bounds]).unwrap(), bounds);
}
}
+ #[test]
+ fn properties_recover_type_from_explicit_unknown_bounds() {
+ let expr = bounds_expr(false);
+ let bounds = Interval::make_unbounded(&DataType::Null).unwrap();
+ let child = ExprProperties::new_unknown().with_range(bounds.clone());
+ assert_eq!(
+ expr.get_properties(&[child]).unwrap().range,
+ Interval::make_unbounded(&DataType::Int32).unwrap()
+ );
+ assert_eq!(expr.evaluate_bounds(&[&bounds]).unwrap(), bounds);
+ }
+
+ #[test]
+ fn properties_propagate_udf_bounds_errors() {
+ let expr = bounds_expr(true);
+ let bounds = Interval::make::<i32>(Some(0), Some(100)).unwrap();
+ let child = ExprProperties::new_unknown().with_range(bounds.clone());
+ assert!(
+ expr.get_properties(&[child])
+ .unwrap_err()
+ .to_string()
+ .contains("bounds evaluation failed")
+ );
+ assert!(
+ expr.evaluate_bounds(&[&bounds])
+ .unwrap_err()
+ .to_string()
+ .contains("bounds evaluation failed")
+ );
+ }
+
#[test]
fn test_scalar_function_volatile_node() {
// Create a volatile UDF
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org