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/21 17:24:03 UTC
[PR] perf: add typed bounds for date_bin and from_unixtime [datafusion]
Toby1009 opened a new pull request, #25575:
URL: https://github.com/apache/datafusion/pull/25575
## Which issue does this PR close?
- Closes #25465.
## Rationale for this change
`date_bin` and `from_unixtime` currently report an unbounded `Null` interval from `evaluate_bounds`. This loses their output type during physical property analysis, so safe timezone-free expressions can retain redundant sorts.
## What changes are included in this PR?
- Return typed unbounded bounds from `date_bin` using its source type.
- Return typed unbounded bounds from `from_unixtime` when its session or explicit timezone is known, while keeping value-dependent unknown timezones unknown.
- Document when scalar UDFs should provide typed unbounded bounds.
- Add plan coverage for redundant sort removal and retain the named-timezone correctness regression case.
## What is the testing strategy for this PR?
Unit tests cover timestamp, timezone-aware timestamp, `Time32`, `Time64`, session timezone, constant explicit timezone, and unknown timezone bounds. SQL logic tests cover redundant sort removal through timezone-free `date_bin` and `from_unixtime`, plus named-timezone correctness.
The following checks pass:
- `cargo fmt --all`
- `cargo clippy --all-targets --all-features -- -D warnings`
- `cargo test -p datafusion-functions evaluate_bounds --lib`
- `cargo test --test sqllogictests -- timestamps.slt`
- The extended workspace test suite required by the contributor guide
## Are there any user-facing changes?
Queries that apply `date_trunc` to timezone-free `date_bin` or `from_unixtime` results can avoid redundant sorts. Named-timezone cases remain conservatively unordered.
--
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: add typed bounds for date_bin and from_unixtime [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #25575:
URL: https://github.com/apache/datafusion/pull/25575#issuecomment-5797397833
A central fallback in `ScalarFunctionExpr::get_properties` makes sense as a follow-up, and I'd keep this PR as is. Columns already get typed unbounded ranges (`equivalence/properties/mod.rs:1618`), so the consumers that read `range.data_type()` (date_trunc, cast, binary time ± interval) already handle typed-unbounded input. A few things to check there:
- keep the `Null` interval if `make_unbounded(return_type)` errors, rather than failing planning
- replace only an unbounded `Null` result, never real bounds from the UDF
- leave `PhysicalExpr::evaluate_bounds` (the cp_solver path) unchanged
- add negative plan tests: a named-timezone function output, and time ± interval through a function
Once that lands, the `date_bin`/`from_unixtime` overrides here can be removed. Agree that `DataType` in `ExprProperties` needs its own design discussion because of the API and FFI surface.
--
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: add typed bounds for date_bin and from_unixtime [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25575:
URL: https://github.com/apache/datafusion/pull/25575
--
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: add typed bounds for date_bin and from_unixtime [datafusion]
Posted by "Toby1009 (via GitHub)" <gi...@apache.org>.
Toby1009 commented on PR #25575:
URL: https://github.com/apache/datafusion/pull/25575#issuecomment-5765234394
While working on this PR, I explored whether preserving output types could be handled centrally in `ScalarFunctionExpr`.
It already stores the resolved `return_field`. When a UDF’s `evaluate_bounds` returns the default unbounded `Null` interval, we could use that resolved type to construct a typed unbounded interval, while preserving any bounds the UDF provides.
In a local prototype, I removed the `date_bin` and `from_unixtime` overrides and added this fallback. The tested timezone-free queries still removed the redundant sort, while explicit and session timezone cases retained it. The Goose Bay regression query also returned the expected ordering. I additionally observed redundant sort removal for `CAST(signum(c) AS REAL)` and `CAST(trunc(c) AS REAL)` over sorted inputs.
This could reduce the need for individual UDFs to implement `evaluate_bounds` solely to preserve type information. However, it would affect property inference for all scalar UDFs, so broader regression testing and checking the behavior for unsupported interval types would be needed.
I also tried storing `DataType` separately in `ExprProperties`. That worked for the datetime cases, but requires migrating consumers and addressing public API and FFI compatibility, so it seems better suited to a separate design discussion.
Would a central fallback be a useful follow-up? My preference is to keep this PR focused and explore that separately if the direction makes sense.
--
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: add typed bounds for date_bin and from_unixtime [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25575:
URL: https://github.com/apache/datafusion/pull/25575#issuecomment-5765537430
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25575?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.42%. Comparing base ([`0576a0b`](https://app.codecov.io/gh/apache/datafusion/commit/0576a0b400437ade5a6f3b102465d954a539f263?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`37e9ab4`](https://app.codecov.io/gh/apache/datafusion/commit/37e9ab44175e93ddfd1ec703636cda1d926df785?dropdown=coverage&el=desc&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 #25575 +/- ##
=======================================
Coverage 82.42% 82.42%
=======================================
Files 1139 1139
Lines 435372 435449 +77
Branches 435372 435449 +77
=======================================
+ Hits 358845 358912 +67
- Misses 54826 54832 +6
- Partials 21701 21705 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25575?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