You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Toby1009 (via GitHub)" <gi...@apache.org> on 2026/09/23 23:59:04 UTC
[PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Toby1009 opened a new pull request, #25668:
URL: https://github.com/apache/datafusion/pull/25668
## Which issue does this PR close?
Closes #25667.
## Rationale for this change
Compositions such as nested `date_trunc` or `CAST(trunc(x) AS REAL)` can retain redundant sorts even when their inputs are ordered. Scalar UDFs using the default bounds implementation lose their output type during property analysis, although `ScalarFunctionExpr` already knows the resolved return type.
This implements the central fallback discussed in [the review of #25575](https://github.com/apache/datafusion/pull/25575#issuecomment-5797397833).
## What changes are included in this PR?
- In `ScalarFunctionExpr::get_properties`, replace an unbounded `Null` result with a typed unbounded interval using the resolved return type. Preserve explicit bounds and UDF evaluation errors; retain the original interval if the fallback type is unsupported.
- Remove the type-only `evaluate_bounds` overrides for `date_bin` and `from_unixtime`, and document the central behavior for UDF authors.
- Keep the `PhysicalExpr::evaluate_bounds` delegation unchanged, without adding a fallback to the constraint solver. Removing the two overrides means their direct bounds evaluation returns the default `Null` interval again.
- Include two equivalent benchmark cleanups required by strict Clippy: a boolean-to-integer conversion and a local type alias.
## What is the testing strategy for this PR?
Unit tests cover resolved output types, explicit and session timezones, unsupported interval types, preservation of existing bounds and errors, and separation from direct bounds evaluation. Removing the fallback makes the new type-recovery tests fail.
SQL tests cover redundant sort removal for nested datetime functions and a numeric UDF followed by a cast, including NULL input. Negative plan and result tests retain the required sorts for named timezone transitions and time-of-day arithmetic across midnight. Existing `date_bin` and `from_unixtime` plan tests remain unchanged.
Validation:
- Extended workspace test suite, including SQL logic tests.
- `cargo fmt --all`.
- `cargo clippy --all-targets --all-features -- -D warnings`.
- `uv run ./dev/rust_lint.sh`.
A local planning experiment against the base `main` commit used the same sorted 100-row `MemTable`, 20 warmups and 200 planning iterations, with three alternating runs of each build. The nested datetime and numeric-cast queries each removed one redundant sort and returned identical results. Median planning times were 679 → 645 µs and 585 → 553 µs; control and struct-return queries were essentially unchanged. These are small local measurements using the unoptimized `ci` profile, not production execution-speed claims.
## Are there any user-facing changes?
Safe compositions of scalar functions can avoid redundant sorts without individual UDFs implementing bounds solely to preserve their output type. Existing ordering rules still determine whether a sort can be removed. No public API signatures or FFI structures change.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Posted by "Toby1009 (via GitHub)" <gi...@apache.org>.
Toby1009 commented on PR #25668:
URL: https://github.com/apache/datafusion/pull/25668#issuecomment-5864367724
@jayzhan211 @viirya @alamb — would you have time to review this?
This implements the central fallback discussed in #25575: preserving resolved scalar UDF output types during property analysis, while keeping explicit bounds and the constraint-solver path unchanged.
Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #25668:
URL: https://github.com/apache/datafusion/pull/25668#discussion_r4147165788
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -377,18 +386,234 @@ mod tests {
}
fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(DataType::Int32)
+ Ok(self.return_type.clone())
}
fn invoke_with_args(&self, _args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- Ok(ColumnarValue::Scalar(ScalarValue::Int32(Some(42))))
+ Ok(ColumnarValue::Scalar(ScalarValue::try_from(
+ &self.return_type,
+ )?))
+ }
+ }
+
+ fn default_bounds_expr(return_type: DataType) -> ScalarFunctionExpr {
+ ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(MockScalarUDF {
+ signature: Signature::exact(vec![], Volatility::Immutable),
+ return_type,
+ })),
+ vec![],
+ &Schema::empty(),
+ Arc::new(ConfigOptions::default()),
+ )
+ .unwrap()
+ }
+
+ #[test]
+ fn properties_recover_return_type_without_changing_bounds_evaluation() {
+ for data_type in [
+ DataType::Null,
+ DataType::Boolean,
+ DataType::Int32,
+ DataType::UInt64,
+ DataType::Float64,
+ DataType::Utf8,
+ DataType::Timestamp(TimeUnit::Second, None),
+ DataType::Timestamp(TimeUnit::Nanosecond, Some("America/Goose_Bay".into())),
+ DataType::Time32(TimeUnit::Millisecond),
+ DataType::Time64(TimeUnit::Microsecond),
+ DataType::List(Arc::new(Field::new("item", DataType::Int32, true))),
+ ] {
+ let expr = default_bounds_expr(data_type.clone());
+ let properties = expr.get_properties(&[]).unwrap();
+ assert_eq!(
+ properties.range,
+ Interval::make_unbounded(&data_type).unwrap()
+ );
+ 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.
Review Comment:
Same wording nit as in `udf.rs`: the cp_solver doesn't accept `ScalarFunctionExpr`, so something like "Direct `evaluate_bounds` calls bypass the fallback" would be more precise.
##########
datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt:
##########
@@ -158,3 +158,58 @@ NULL
statement ok
DROP TABLE time_vals
+
+# Time arithmetic can wrap even when its input is an ordered function result.
Review Comment:
I don't think these two queries can catch a regression in the fallback. SQL-planned `BinaryExpr`s have `fail_on_overflow = false`, and a function output's range is unbounded, so `arithmetic_sort_properties` returns `Unordered` through the overflow rule whether the left range is `Time64` or `Null`. The sort stays either way, including with the fallback line removed.
The `wraps_in_domain` check, which is the part that actually depends on the recovered type, is only reached with `with_fail_on_overflow(true)`. Could you add a unit test that builds `date_trunc('hour', t) + INTERVAL '2 hours'` as a `BinaryExpr` with `with_fail_on_overflow(true)` over an ordered `Time64` child and asserts `Unordered`? The SLTs are still fine to keep as end-to-end result checks.
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -377,18 +386,234 @@ mod tests {
}
fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(DataType::Int32)
+ Ok(self.return_type.clone())
}
fn invoke_with_args(&self, _args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- Ok(ColumnarValue::Scalar(ScalarValue::Int32(Some(42))))
+ Ok(ColumnarValue::Scalar(ScalarValue::try_from(
+ &self.return_type,
+ )?))
+ }
+ }
+
+ fn default_bounds_expr(return_type: DataType) -> ScalarFunctionExpr {
+ ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(MockScalarUDF {
+ signature: Signature::exact(vec![], Volatility::Immutable),
+ return_type,
+ })),
+ vec![],
+ &Schema::empty(),
+ Arc::new(ConfigOptions::default()),
+ )
+ .unwrap()
+ }
+
+ #[test]
+ fn properties_recover_return_type_without_changing_bounds_evaluation() {
+ for data_type in [
+ DataType::Null,
+ DataType::Boolean,
+ DataType::Int32,
+ DataType::UInt64,
+ DataType::Float64,
+ DataType::Utf8,
+ DataType::Timestamp(TimeUnit::Second, None),
+ DataType::Timestamp(TimeUnit::Nanosecond, Some("America/Goose_Bay".into())),
+ DataType::Time32(TimeUnit::Millisecond),
+ DataType::Time64(TimeUnit::Microsecond),
+ DataType::List(Arc::new(Field::new("item", DataType::Int32, true))),
+ ] {
+ let expr = default_bounds_expr(data_type.clone());
+ let properties = expr.get_properties(&[]).unwrap();
+ assert_eq!(
+ properties.range,
+ Interval::make_unbounded(&data_type).unwrap()
+ );
+ 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.
+ assert_eq!(
+ expr.evaluate_bounds(&[]).unwrap(),
+ Interval::make_unbounded(&DataType::Null).unwrap()
+ );
+ }
+ }
+
+ #[test]
+ fn properties_keep_unknown_bounds_for_unsupported_return_type() {
+ // This unit is not supported for Time32 by ScalarValue/Interval.
+ let data_type = DataType::Time32(TimeUnit::Nanosecond);
+ assert!(Interval::make_unbounded(&data_type).is_err());
+ let properties = default_bounds_expr(data_type).get_properties(&[]).unwrap();
+ assert_eq!(
+ properties.range,
+ Interval::make_unbounded(&DataType::Null).unwrap()
+ );
+ }
+
+ #[test]
+ fn datetime_properties_use_resolved_return_types() {
+ use datafusion_functions::datetime::date_bin::DateBinFunc;
+ use datafusion_functions::datetime::from_unixtime::FromUnixtimeFunc;
+
+ for source_type in [
+ DataType::Timestamp(TimeUnit::Second, None),
+ DataType::Timestamp(TimeUnit::Nanosecond, Some("UTC".into())),
+ DataType::Time32(TimeUnit::Millisecond),
+ DataType::Time64(TimeUnit::Microsecond),
+ ] {
+ let expr = ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(DateBinFunc::new())),
+ vec![
+ Arc::new(Literal::new(ScalarValue::new_interval_mdn(
+ 0,
+ 0,
+ 60_000_000_000,
+ ))),
+ Arc::new(Column::new("ts", 0)),
+ ],
+ &Schema::new(vec![Field::new("ts", source_type.clone(), true)]),
+ Arc::new(ConfigOptions::default()),
+ )
+ .unwrap();
+ let properties = expr
+ .get_properties(&[
+ ExprProperties::new_unknown().with_order(SortProperties::Singleton),
+ ExprProperties::new_unknown()
+ .with_range(Interval::make_unbounded(&source_type).unwrap()),
+ ])
+ .unwrap();
+ assert_eq!(
+ properties.range,
+ Interval::make_unbounded(&source_type).unwrap()
+ );
+ }
+
+ for (session_tz, explicit_tz, expected_tz) in [
+ (None, None, None),
+ (Some("America/Denver"), None, Some("America/Denver")),
+ (
+ Some("America/Denver"),
+ Some("America/Goose_Bay"),
+ Some("America/Goose_Bay"),
+ ),
+ ] {
+ let mut config = ConfigOptions::default();
+ config.execution.time_zone = session_tz.map(str::to_owned);
+ let udf = FromUnixtimeFunc::new_with_config(&config);
+ let mut args: Vec<Arc<dyn PhysicalExpr>> =
+ vec![Arc::new(Column::new("c", 0))];
+ if let Some(tz) = explicit_tz {
+ args.push(Arc::new(Literal::new(ScalarValue::from(tz))));
+ }
+ let children = vec![ExprProperties::new_unknown(); args.len()];
+ let expr = ScalarFunctionExpr::try_new(
+ Arc::new(ScalarUDF::from(udf)),
+ args,
+ &Schema::new(vec![Field::new("c", DataType::Int64, true)]),
+ Arc::new(config),
+ )
+ .unwrap();
+ // The literal's timezone was resolved during construction, even
+ // though the child intervals here contain no value information.
+ let expected =
+ DataType::Timestamp(TimeUnit::Second, expected_tz.map(Into::into));
+ assert_eq!(
+ expr.get_properties(&children).unwrap().range,
+ Interval::make_unbounded(&expected).unwrap()
+ );
+ }
+ }
+
+ #[derive(Debug, PartialEq, Eq, Hash)]
+ struct BoundsUDF {
+ inner: MockScalarUDF,
+ fail: bool,
+ }
+
+ impl ScalarUDFImpl for BoundsUDF {
+ fn name(&self) -> &str {
+ "bounds_function"
+ }
+
+ fn signature(&self) -> &Signature {
+ self.inner.signature()
+ }
+
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ self.inner.return_type(arg_types)
+ }
+
+ fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
+ self.inner.invoke_with_args(args)
+ }
+
+ fn evaluate_bounds(&self, inputs: &[&Interval]) -> Result<Interval> {
+ if self.fail {
+ return internal_err!("bounds evaluation failed");
+ }
+ Ok(inputs[0].clone())
+ }
+ }
+
+ #[test]
+ fn properties_preserve_udf_bounds_and_errors() {
+ for fail in [false, true] {
Review Comment:
nit: the `fail` / non-`fail` branches share little besides setup; two separate tests (bounds preserved, error propagated) would read more easily than the loop plus `if fail`.
##########
datafusion/expr/src/udf.rs:
##########
@@ -908,15 +908,15 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any {
/// # Output type
///
/// The default implementation returns an unbounded [`DataType::Null`]
- /// interval because the output type cannot be inferred generically.
- /// Implementations should override this method when they can determine the
- /// output type, even if they cannot compute precise bounds. Returning an
- /// unbounded interval with the correct type preserves information used by
- /// downstream physical property analysis.
- ///
- /// If the output type depends on argument values, only report it when those
- /// values are known, such as from singleton input intervals. Otherwise,
- /// keep the output type unknown.
+ /// 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
Review Comment:
Two small wording suggestions:
- "direct bounds evaluation by the constraint solver": `check_support` in `physical-expr/src/intervals/utils.rs` doesn't accept `ScalarFunctionExpr`, so the cp_solver never reaches this path. Maybe "direct calls to `PhysicalExpr::evaluate_bounds`" is more accurate.
- "Explicit bounds are preserved": it may help to spell out the exact rule, i.e. *any* unbounded `Null` result is replaced (including one an override returns on purpose), and any other interval is used as-is, even if its type differs from the resolved return type.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25668:
URL: https://github.com/apache/datafusion/pull/25668#issuecomment-5805190549
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25668?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.88235%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.49%. Comparing base ([`95bb0a0`](https://app.codecov.io/gh/apache/datafusion/commit/95bb0a0dfa48ca20d017faa55b2fc96ee2f300fe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`00daf02`](https://app.codecov.io/gh/apache/datafusion/commit/00daf021f996524b9616336d061c2c70b3ef441d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25668?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-expr/src/scalar\_function.rs](https://app.codecov.io/gh/apache/datafusion/pull/25668?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fscalar_function.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9zY2FsYXJfZnVuY3Rpb24ucnM=) | 95.88% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25668 +/- ##
=======================================
Coverage 82.48% 82.49%
=======================================
Files 1140 1140
Lines 437614 437706 +92
Branches 437614 437706 +92
=======================================
+ Hits 360986 361064 +78
- Misses 54834 54848 +14
Partials 21794 21794
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25668?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25668:
URL: https://github.com/apache/datafusion/pull/25668#issuecomment-5915500750
I merged up to get the most recent tests / runs so we can get this PR in
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: preserve scalar UDF output types in property analysis [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25668:
URL: https://github.com/apache/datafusion/pull/25668
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org