You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/06/28 18:50:11 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6789: Minor: Make `date_trunc` code easier to understand
alamb opened a new pull request, #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789
# Which issue does this PR close?
Related to to https://github.com/apache/arrow-datafusion/issues/6653 and https://github.com/apache/arrow-datafusion/pull/6654
# Rationale for this change
Both @viirya and I were confused about a seeming special case handling of `microsecond` and `millisecond` truncation in the `date_trunc` code -- see https://github.com/apache/arrow-datafusion/pull/6654#discussion_r1244324968
# What changes are included in this PR?
Update the code so `microsecond` and `millisecond` are handled the same way as the other granularities
# Are these changes tested?
Covered by existing tests
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Weijun-H commented on pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789#issuecomment-1612116013
LGTM. Nice refactoring !👍
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789#issuecomment-1611917225
cc @Weijun-H and @viirya
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789#discussion_r1245626134
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -219,42 +219,49 @@ fn quarter_month(date: &NaiveDateTime) -> u32 {
/// account that some granularities are not uniform durations of time
/// (e.g. months are not always the same lengths, leap seconds, etc)
fn date_trunc_coarse(granularity: &str, value: i64) -> Result<i64> {
- if granularity == "millisecond" || granularity == "microsecond" {
Review Comment:
this special case was what is confusing. I removed this one and inlined the `with_nanosecond(0)` call for all the other cases
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789#discussion_r1245691119
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -979,6 +988,7 @@ mod tests {
];
cases.iter().for_each(|(original, granularity, expected)| {
+ println!("original: {original}, granularity: {granularity}, expected: {expected:?}");
Review Comment:
```suggestion
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6789: Minor: Make `date_trunc` code easier to understand
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6789:
URL: https://github.com/apache/arrow-datafusion/pull/6789#discussion_r1245691545
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -219,42 +219,51 @@ fn quarter_month(date: &NaiveDateTime) -> u32 {
/// account that some granularities are not uniform durations of time
/// (e.g. months are not always the same lengths, leap seconds, etc)
fn date_trunc_coarse(granularity: &str, value: i64) -> Result<i64> {
- if granularity == "millisecond" || granularity == "microsecond" {
- return Ok(value);
- }
+ // Use chrono NaiveDateTime to clear the various fields
+ // correctly accounting for non uniform granularities
+ let value = timestamp_ns_to_datetime(value).ok_or_else(|| {
+ DataFusionError::Execution(format!("Timestamp {value} out of range"))
+ })?;
+
+ let value = Some(value);
- let value = timestamp_ns_to_datetime(value)
- .ok_or_else(|| {
- DataFusionError::Execution(format!("Timestamp {value} out of range"))
- })?
- .with_nanosecond(0);
let value = match granularity {
- "second" => value,
- "minute" => value.and_then(|d| d.with_second(0)),
+ "millisecond" => value,
+ "microsecond" => value,
+ "second" => value.and_then(|d| d.with_nanosecond(0)),
Review Comment:
Yea, it is more clear.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org