You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "namanjain24-sudo (via GitHub)" <gi...@apache.org> on 2026/09/19 10:28:02 UTC
[PR] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
namanjain24-sudo opened a new pull request, #25498:
URL: https://github.com/apache/datafusion/pull/25498
## Which issue does this PR close?
- Closes #25432.
## Rationale for this change
`process_scalar` always went through `general_date_trunc`, which converts the value to nanoseconds before truncating. A `Timestamp(Second)`, `Timestamp(Millisecond)` or `Timestamp(Microsecond)` value outside the nanosecond range (after 2262 or before 1677) therefore failed with `Timestamp ... out of range`.
`process_array` does not convert for the fine granularities: when `granularity.is_fine_granularity()`, or the value has no timezone and the granularity is `hour` or `day`, it truncates in the input's own unit with plain arithmetic. So the same value succeeded in a column and failed as a scalar:
```sql
SELECT date_trunc('second', to_timestamp_seconds(10000000000));
-- Execution error: Timestamp 10000000000 out of range
SELECT date_trunc('second', to_timestamp_seconds(ts)) FROM (VALUES (10000000000)) AS t(ts);
-- 2286-11-20T17:46:40
```
## What changes are included in this PR?
The scalar path now takes the same route as the array path:
- `truncates_in_input_unit` is the condition `process_array` already used, now shared by both paths.
- `fine_granularity_unit` is the unit table from `general_date_trunc_array_fine_granularity`, moved out so both paths read it.
- `date_trunc_fine_granularity` truncates one value in its own unit, with the checked subtraction and error message the array path already used for values near `i64::MIN`. The array path's slow branch now calls it too.
For those granularities, a scalar and a one-row column now run the same function, so they accept the same values and give the same result. Everything else (`week`, `month`, `quarter`, `year`, and `hour`/`day` with a timezone) still goes through `general_date_trunc` on both paths, so it is unchanged and still limited to the nanosecond range for both scalars and columns.
One existing test changes. `date_trunc('hour', arrow_cast(9223372036854775807, 'Timestamp(Second, None)'))` was expected to fail with `Timestamp 9223372036854775807 out of range`. That test comes from #22262, whose goal was an error instead of a panic, and a column holding that value already succeeds. As a scalar it now succeeds too, so the test uses `week`, which still converts to nanoseconds on both paths and so still covers the error.
## Are these changes tested?
- New unit test `scalar_and_array_accept_timestamps_beyond_nanosecond_range` evaluates `date_trunc` on a scalar and on a one-row column with the same value, for all three coarser-than-nanosecond units, with and without a timezone, and for values before 1677 and after 2262, and checks that both succeed with the same result.
- New sqllogictest queries in `datetime/timestamps.slt` cover the issue's scalar and column queries, plus millisecond and microsecond scalars.
- With the scalar change disabled, the new unit test fails, and the three new scalar queries fail with the error from the issue (`Timestamp 10000000000 out of range`, etc.), while the column query passes as it does on `main`.
- `cargo test -p datafusion-functions` (384 passed), the full sqllogictest suite (520 files), `cargo fmt` and `cargo clippy -p datafusion-functions --all-targets --all-features -- -D warnings` pass.
## Are there any user-facing changes?
`date_trunc` on a scalar timestamp outside the nanosecond range now returns the same result as on a column, instead of an out-of-range error, for `microsecond`, `millisecond`, `second` and `minute`, and for `hour` and `day` without a timezone. No API changes.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4054294454
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -1441,6 +1472,96 @@ mod tests {
}
}
+ /// Evaluates `date_trunc(granularity, value)` once on a scalar and once on a
+ /// one-row column holding the same value.
+ fn date_trunc_scalar_and_array(
+ granularity: &str,
+ value: ScalarValue,
+ ) -> (
+ datafusion_common::Result<ScalarValue>,
+ datafusion_common::Result<ScalarValue>,
+ ) {
+ let invoke = |arg: ColumnarValue| {
+ let data_type = value.data_type();
+ let args = ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(ScalarValue::from(granularity)), arg],
+ arg_fields: vec![
+ Field::new("a", DataType::Utf8, false).into(),
+ Field::new("b", data_type.clone(), true).into(),
+ ],
+ number_rows: 1,
+ return_field: Field::new("f", data_type, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ match DateTruncFunc::new().invoke_with_args(args)? {
+ ColumnarValue::Scalar(result) => Ok(result),
+ ColumnarValue::Array(result) => ScalarValue::try_from_array(&result, 0),
+ }
+ };
+ (
+ invoke(ColumnarValue::Scalar(value.clone())),
+ invoke(ColumnarValue::Array(value.to_array().unwrap())),
+ )
+ }
+
+ /// A timestamp beyond the nanosecond range is truncated the same way as a
+ /// scalar and as a column: neither converts it to nanoseconds first.
+ #[test]
+ fn scalar_and_array_accept_timestamps_beyond_nanosecond_range() {
+ // 2286-11-20T17:46:40, after the last nanosecond timestamp in 2262
+ let seconds = 10_000_000_000;
+ let utc: Option<Arc<str>> = Some("UTC".into());
+ let cases = [
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), None),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 1), None),
+ "hour",
+ ),
+ (ScalarValue::TimestampSecond(Some(seconds + 1), None), "day"),
+ (ScalarValue::TimestampSecond(Some(seconds), None), "second"),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), utc.clone()),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampMillisecond(Some(seconds * 1_000 + 999), None),
+ "second",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(seconds * 1_000_000 + 999), None),
+ "millisecond",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(-seconds * 1_000_000 - 1), utc),
+ "second",
+ ),
+ ];
+ for (value, granularity) in cases {
+ let (scalar, array) = date_trunc_scalar_and_array(granularity, value.clone());
+ let scalar = scalar.unwrap_or_else(|e| {
+ panic!("scalar date_trunc('{granularity}', {value:?}) failed: {e}")
+ });
+ assert_eq!(
+ scalar,
+ array.unwrap(),
+ "date_trunc('{granularity}', {value:?})"
+ );
+ }
+
+ // The issue's example: `to_timestamp_seconds(10000000000)`
Review Comment:
Fair point. I reworded it to say what it checks and added the issue link, the same way other comments under `datetime/` link theirs.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4054289298
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -796,13 +772,8 @@ fn general_date_trunc_array_fine_granularity<T: ArrowTimestampType>(
})
.collect();
let array: PrimitiveArray<T> = if maybe_underflow {
- array.try_unary(|value| {
- value.checked_sub(value.rem_euclid(unit)).ok_or_else(|| {
- exec_datafusion_err!(
- "Timestamp {value} out of range after truncating to {granularity}"
- )
- })
- })?
+ array
+ .try_unary(|value| date_trunc_fine_granularity(tu, value, granularity))?
Review Comment:
Good catch, agreed. In 5c830eb the slow branch now passes the `unit` it already has to a small `truncate_to_unit(value, unit, granularity)`, and the scalar path looks the unit up once, so `date_trunc_fine_granularity` is gone.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "benbellick (via GitHub)" <gi...@apache.org>.
benbellick commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4057571566
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -1441,6 +1468,98 @@ mod tests {
}
}
+ /// Evaluates `date_trunc(granularity, value)` once on a scalar and once on a
+ /// one-row column holding the same value.
+ fn date_trunc_scalar_and_array(
+ granularity: &str,
+ value: ScalarValue,
+ ) -> (
+ datafusion_common::Result<ScalarValue>,
+ datafusion_common::Result<ScalarValue>,
+ ) {
+ let invoke = |arg: ColumnarValue| {
+ let data_type = value.data_type();
+ let args = ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(ScalarValue::from(granularity)), arg],
+ arg_fields: vec![
+ Field::new("a", DataType::Utf8, false).into(),
+ Field::new("b", data_type.clone(), true).into(),
+ ],
+ number_rows: 1,
+ return_field: Field::new("f", data_type, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ match DateTruncFunc::new().invoke_with_args(args)? {
+ ColumnarValue::Scalar(result) => Ok(result),
+ ColumnarValue::Array(result) => ScalarValue::try_from_array(&result, 0),
+ }
+ };
+ (
+ invoke(ColumnarValue::Scalar(value.clone())),
+ invoke(ColumnarValue::Array(value.to_array().unwrap())),
+ )
+ }
+
+ /// A timestamp beyond the nanosecond range is truncated the same way as a
+ /// scalar and as a column: neither converts it to nanoseconds first.
+ #[test]
+ fn scalar_and_array_accept_timestamps_beyond_nanosecond_range() {
+ // 2286-11-20T17:46:40, after the last nanosecond timestamp in 2262
+ let seconds = 10_000_000_000;
+ let utc: Option<Arc<str>> = Some("UTC".into());
+ let cases = [
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), None),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 1), None),
+ "hour",
+ ),
+ (ScalarValue::TimestampSecond(Some(seconds + 1), None), "day"),
+ (ScalarValue::TimestampSecond(Some(seconds), None), "second"),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), utc.clone()),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampMillisecond(Some(seconds * 1_000 + 999), None),
+ "second",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(seconds * 1_000_000 + 999), None),
+ "millisecond",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(-seconds * 1_000_000 - 1), utc),
+ "second",
+ ),
+ ];
Review Comment:
Great tests! I think they could benefit from some comments or some named comments to make them a bit easier to recognize their intent on a quick glance.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "Toby1009 (via GitHub)" <gi...@apache.org>.
Toby1009 commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5779664390
LGTM! I also checked underflow through both the scalar and array paths locally.
One optional test suggestion: could we add a `TimestampSecond(i64::MIN)` / `minute` case using `date_trunc_scalar_and_array`, asserting that both return an error? The existing `scalar_fine_granularity_rejects_underflow` test calls `general_date_trunc` directly, so it doesn't exercise the new scalar fast path. Non-blocking.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "benbellick (via GitHub)" <gi...@apache.org>.
benbellick commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5877193726
yup still LGTM!
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "benbellick (via GitHub)" <gi...@apache.org>.
benbellick commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4054224294
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
Review Comment:
worth seeing if there is a regression. Have you tried the `date_trunc_minute_1000` benchmark?
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -796,13 +772,8 @@ fn general_date_trunc_array_fine_granularity<T: ArrowTimestampType>(
})
.collect();
let array: PrimitiveArray<T> = if maybe_underflow {
- array.try_unary(|value| {
- value.checked_sub(value.rem_euclid(unit)).ok_or_else(|| {
- exec_datafusion_err!(
- "Timestamp {value} out of range after truncating to {granularity}"
- )
- })
- })?
+ array
+ .try_unary(|value| date_trunc_fine_granularity(tu, value, granularity))?
Review Comment:
IIUC we above do some work to turn `tu` into `unit` via `fine_granularity_unit` and then do the same computation again inside of this function `date_trunc_fine_granularity`. That feels like:
1. duplicated work, and
2. a potential source of confusion while reading the code
I think it is worth coming up with a simplification. What you think?
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs merged PR #25498:
URL: https://github.com/apache/datafusion/pull/25498
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5853478131
@benbellick added the underflow test Toby1009 suggested since your LGTM — could you take one more quick look when you get a chance?
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4062452547
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -1441,6 +1468,98 @@ mod tests {
}
}
+ /// Evaluates `date_trunc(granularity, value)` once on a scalar and once on a
+ /// one-row column holding the same value.
+ fn date_trunc_scalar_and_array(
+ granularity: &str,
+ value: ScalarValue,
+ ) -> (
+ datafusion_common::Result<ScalarValue>,
+ datafusion_common::Result<ScalarValue>,
+ ) {
+ let invoke = |arg: ColumnarValue| {
+ let data_type = value.data_type();
+ let args = ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(ScalarValue::from(granularity)), arg],
+ arg_fields: vec![
+ Field::new("a", DataType::Utf8, false).into(),
+ Field::new("b", data_type.clone(), true).into(),
+ ],
+ number_rows: 1,
+ return_field: Field::new("f", data_type, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ match DateTruncFunc::new().invoke_with_args(args)? {
+ ColumnarValue::Scalar(result) => Ok(result),
+ ColumnarValue::Array(result) => ScalarValue::try_from_array(&result, 0),
+ }
+ };
+ (
+ invoke(ColumnarValue::Scalar(value.clone())),
+ invoke(ColumnarValue::Array(value.to_array().unwrap())),
+ )
+ }
+
+ /// A timestamp beyond the nanosecond range is truncated the same way as a
+ /// scalar and as a column: neither converts it to nanoseconds first.
+ #[test]
+ fn scalar_and_array_accept_timestamps_beyond_nanosecond_range() {
+ // 2286-11-20T17:46:40, after the last nanosecond timestamp in 2262
+ let seconds = 10_000_000_000;
+ let utc: Option<Arc<str>> = Some("UTC".into());
+ let cases = [
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), None),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 1), None),
+ "hour",
+ ),
+ (ScalarValue::TimestampSecond(Some(seconds + 1), None), "day"),
+ (ScalarValue::TimestampSecond(Some(seconds), None), "second"),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), utc.clone()),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampMillisecond(Some(seconds * 1_000 + 999), None),
+ "second",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(seconds * 1_000_000 + 999), None),
+ "millisecond",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(-seconds * 1_000_000 - 1), utc),
+ "second",
+ ),
+ ];
Review Comment:
Thanks, agreed. In 6a213a76f each group of cases in `scalar_and_array_accept_timestamps_beyond_nanosecond_range` now has a short comment saying what it covers (the input unit and granularity, the case where the granularity is the input's own unit, the time zone, and the pre-epoch value). It's comments only, no change to the cases or the assertions. I ran the `date_trunc` unit tests, `cargo fmt --check` and clippy on `datafusion-functions` locally.
--
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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "benbellick (via GitHub)" <gi...@apache.org>.
benbellick commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5763026913
cc @gabotechs would you mind taking a look at this one if you have a moment? It is useful for the work I am doing in https://github.com/apache/datafusion/issues/25345 to optimize aggregations on range partitions by skipping the repartition when possible. 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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5741205585
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25498?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.00000%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.38%. Comparing base ([`ccfe704`](https://app.codecov.io/gh/apache/datafusion/commit/ccfe70480630762da96c2698bceaad9d919b786c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`408c5be`](https://app.codecov.io/gh/apache/datafusion/commit/408c5be194473274f2a89375d514c372158895b4?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/25498?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/functions/src/datetime/date\_trunc.rs](https://app.codecov.io/gh/apache/datafusion/pull/25498?src=pr&el=tree&filepath=datafusion%2Ffunctions%2Fsrc%2Fdatetime%2Fdate_trunc.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mdW5jdGlvbnMvc3JjL2RhdGV0aW1lL2RhdGVfdHJ1bmMucnM=) | 95.00% | [3 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25498?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 #25498 +/- ##
==========================================
- Coverage 82.38% 82.38% -0.01%
==========================================
Files 1138 1138
Lines 433733 433823 +90
Branches 433733 433823 +90
==========================================
+ Hits 357331 357404 +73
- Misses 54848 54859 +11
- Partials 21554 21560 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25498?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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "benbellick (via GitHub)" <gi...@apache.org>.
benbellick commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4054239396
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -1441,6 +1472,96 @@ mod tests {
}
}
+ /// Evaluates `date_trunc(granularity, value)` once on a scalar and once on a
+ /// one-row column holding the same value.
+ fn date_trunc_scalar_and_array(
+ granularity: &str,
+ value: ScalarValue,
+ ) -> (
+ datafusion_common::Result<ScalarValue>,
+ datafusion_common::Result<ScalarValue>,
+ ) {
+ let invoke = |arg: ColumnarValue| {
+ let data_type = value.data_type();
+ let args = ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(ScalarValue::from(granularity)), arg],
+ arg_fields: vec![
+ Field::new("a", DataType::Utf8, false).into(),
+ Field::new("b", data_type.clone(), true).into(),
+ ],
+ number_rows: 1,
+ return_field: Field::new("f", data_type, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ match DateTruncFunc::new().invoke_with_args(args)? {
+ ColumnarValue::Scalar(result) => Ok(result),
+ ColumnarValue::Array(result) => ScalarValue::try_from_array(&result, 0),
+ }
+ };
+ (
+ invoke(ColumnarValue::Scalar(value.clone())),
+ invoke(ColumnarValue::Array(value.to_array().unwrap())),
+ )
+ }
+
+ /// A timestamp beyond the nanosecond range is truncated the same way as a
+ /// scalar and as a column: neither converts it to nanoseconds first.
+ #[test]
+ fn scalar_and_array_accept_timestamps_beyond_nanosecond_range() {
+ // 2286-11-20T17:46:40, after the last nanosecond timestamp in 2262
+ let seconds = 10_000_000_000;
+ let utc: Option<Arc<str>> = Some("UTC".into());
+ let cases = [
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), None),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 1), None),
+ "hour",
+ ),
+ (ScalarValue::TimestampSecond(Some(seconds + 1), None), "day"),
+ (ScalarValue::TimestampSecond(Some(seconds), None), "second"),
+ (
+ ScalarValue::TimestampSecond(Some(seconds + 59), utc.clone()),
+ "minute",
+ ),
+ (
+ ScalarValue::TimestampMillisecond(Some(seconds * 1_000 + 999), None),
+ "second",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(seconds * 1_000_000 + 999), None),
+ "millisecond",
+ ),
+ (
+ ScalarValue::TimestampMicrosecond(Some(-seconds * 1_000_000 - 1), utc),
+ "second",
+ ),
+ ];
+ for (value, granularity) in cases {
+ let (scalar, array) = date_trunc_scalar_and_array(granularity, value.clone());
+ let scalar = scalar.unwrap_or_else(|e| {
+ panic!("scalar date_trunc('{granularity}', {value:?}) failed: {e}")
+ });
+ assert_eq!(
+ scalar,
+ array.unwrap(),
+ "date_trunc('{granularity}', {value:?})"
+ );
+ }
+
+ // The issue's example: `to_timestamp_seconds(10000000000)`
Review Comment:
This isn't going to make sense as a comment once this is merged. I would either include the issue number for future reference or get rid of the mention of the issue, whatever is the standard in this repo (I'm not sure). 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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#issuecomment-5780891126
Good catch, added `scalar_and_array_reject_fine_granularity_underflow` using `date_trunc_scalar_and_array` with `TimestampSecond(i64::MIN)`/`minute`, asserting both paths error. Confirmed it goes through the scalar fast path (`process_scalar`) rather than `general_date_trunc`. 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] fix: truncate scalar timestamps in their own unit in date_trunc [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4054292728
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
Review Comment:
Ran it against the branch's base (ccfe704) on the same machine, together with the other `date_trunc` benches: `date_trunc_minute_1000` went from 1.246 µs to 1.232 µs, and the rest are within noise. That matches the change, since the array hot loop is the same and only the unit table moved into a helper.
--
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