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/27 21:11:01 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6783: Minor: Simplify `date_trunc` code and add comments
alamb opened a new pull request, #6783:
URL: https://github.com/apache/arrow-datafusion/pull/6783
# Which issue does this PR close?
Follow on to https://github.com/apache/arrow-datafusion/pull/6654 from @Weijun-H (❤️ )
# Rationale for this change
While reviewing https://github.com/apache/arrow-datafusion/pull/6654 and trying to answer @viirya 's question here https://github.com/apache/arrow-datafusion/pull/6654#discussion_r1244324968
# What changes are included in this PR?
1. Rename `date_trunc_single` to `date_trunc_coarse` to better explain what it does
2. Add comments to explain how it works
3. Remove the use of indirection and call the function directly
# Are these changes tested?
Covered by the (very) nice existing tests added in https://github.com/apache/arrow-datafusion/pull/6654
# Are there any user-facing changes?
No
--
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 #6783: Minor: Simplify `date_trunc` code and add comments
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6783:
URL: https://github.com/apache/arrow-datafusion/pull/6783
--
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 a diff in pull request #6783: Minor: Simplify `date_trunc` code and add comments
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #6783:
URL: https://github.com/apache/arrow-datafusion/pull/6783#discussion_r1244843770
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -214,7 +214,11 @@ fn quarter_month(date: &NaiveDateTime) -> u32 {
1 + 3 * ((date.month() - 1) / 3)
}
-fn date_trunc_single(granularity: &str, value: i64) -> Result<i64> {
+/// Tuncates the single `value`, expressed in nanoseconds since the
Review Comment:
Nice simplification 👍 !
--
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 #6783: Minor: Simplify `date_trunc` code and add comments
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6783:
URL: https://github.com/apache/arrow-datafusion/pull/6783#discussion_r1244365684
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -214,7 +214,11 @@ fn quarter_month(date: &NaiveDateTime) -> u32 {
1 + 3 * ((date.month() - 1) / 3)
}
-fn date_trunc_single(granularity: &str, value: i64) -> Result<i64> {
+/// Tuncates the single `value`, expressed in nanoseconds since the
Review Comment:
I think this is correct -- @Weijun-H does it match your understanding?
--
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 #6783: Minor: Simplify `date_trunc` code and add comments
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6783:
URL: https://github.com/apache/arrow-datafusion/pull/6783#discussion_r1244365027
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -284,9 +288,7 @@ fn _date_trunc(
};
// convert to nanoseconds
- let Some(nano) = (f)(Some(value * scale))? else {
- return Ok(None);
- };
+ let nano = date_trunc_coarse(granularity, scale * value)?;
Review Comment:
There is no need for a closure, and the function can be called directly which I think makes things easier to understand
--
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