You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/10 09:11:00 UTC
[GitHub] [arrow-rs] MazterQyou opened a new pull request, #1836: Add `quarter` support in `temporal`
MazterQyou opened a new pull request, #1836:
URL: https://github.com/apache/arrow-rs/pull/1836
# Which issue does this PR close?
Closes #1835.
# Rationale for this change
Getting date's quarter is widely supported across different RDBMS and is trivial to implement.
This allows, for instance, [extending apache/arrow-datafusion's `date_part` SQL builtin](https://github.com/apache/arrow-datafusion/blob/67d91a7f1f26a795966c4cc0b200187778ee840c/datafusion/physical-expr/src/datetime_expressions.rs#L352) to support `quarter` field extraction.
# What changes are included in this PR?
This PR adds a `quarter` extractor to `temporal`, and a few related tests.
# Are there any user-facing 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] MazterQyou commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
MazterQyou commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894966007
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -112,6 +112,32 @@ macro_rules! return_compute_error_with {
};
}
+trait ChronoDateQuarter {
+ fn quarter(&self) -> u32;
+
+ fn quarter0(&self) -> u32;
Review Comment:
There's no particular reason to have quarter0 at the moment, but `chrono` implements those `0` getters so I followd. I thought it might be more future-proof that way, besides the fact one could be based on another.
If you think `quarter0` should not be there because it serve no purpose at the moment, I can refactor the trait to have only `quarter` fn.
--
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-rs] MazterQyou commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
MazterQyou commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894966737
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -183,6 +209,34 @@ where
Ok(b.finish())
}
+/// Extracts the quarter of a given temporal array as an array of integers
+pub fn quarter<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
Review Comment:
Indeed. This is regarding `date_trunc` function though, while `date_part` in DataFusion, for instance, uses `arrow-rs`'s `temporal`.
FYI I had an attempt to bring `quarter` to `date_part` in DF without adding it to `arrow-rs` first, but this required a refactor of the macro and made things look weird, while having `quarter` here is a nice addition and makes the same addition in DF simple and elegant rather than hacky.
--
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-rs] tustvold commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894712182
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -389,6 +443,48 @@ mod tests {
assert_eq!(2012, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date64_quarter() {
+ //1514764800000 -> 2018-01-01
+ //1566275025000 -> 2019-08-20
Review Comment:
Is it worth checking a month on the leading edge, e.g. first day of April
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -112,6 +112,32 @@ macro_rules! return_compute_error_with {
};
}
+trait ChronoDateQuarter {
+ fn quarter(&self) -> u32;
+
+ fn quarter0(&self) -> u32;
Review Comment:
Is there a particular reason to have quarter0?
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -112,6 +112,32 @@ macro_rules! return_compute_error_with {
};
}
+trait ChronoDateQuarter {
+ fn quarter(&self) -> u32;
Review Comment:
```suggestion
/// Returns a value from `1..=4` indicating the quarter this date falls into
fn quarter(&self) -> u32;
```
--
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-rs] MazterQyou closed pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
MazterQyou closed pull request #1836: Add `quarter` support in `temporal`
URL: https://github.com/apache/arrow-rs/pull/1836
--
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-rs] codecov-commenter commented on pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#issuecomment-1152166968
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1836](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ccc7fb4) into [master](https://codecov.io/gh/apache/arrow-rs/commit/bd334895439f032d9755b54abf3166d1c8acd7ec?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bd33489) will **increase** coverage by `0.00%`.
> The diff coverage is `87.50%`.
> :exclamation: Current head ccc7fb4 differs from pull request most recent head d905c47. Consider uploading reports for the commit d905c47 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1836 +/- ##
=======================================
Coverage 83.48% 83.48%
=======================================
Files 201 201
Lines 56838 56878 +40
=======================================
+ Hits 47451 47485 +34
- Misses 9387 9393 +6
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/temporal.rs](https://codecov.io/gh/apache/arrow-rs/pull/1836/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy90ZW1wb3JhbC5ycw==) | `95.77% <87.50%> (-1.36%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1836/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.46% <0.00%> (-0.20%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bd33489...d905c47](https://codecov.io/gh/apache/arrow-rs/pull/1836?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-rs] MazterQyou commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
MazterQyou commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894965830
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -389,6 +443,48 @@ mod tests {
assert_eq!(2012, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date64_quarter() {
+ //1514764800000 -> 2018-01-01
+ //1566275025000 -> 2019-08-20
Review Comment:
I don't think it's worth it considering the first test is already at the beginning of a quarter.
If you think it's a good idea to test that specifically, I could either change the first test to April 1st, or add another date to the test.
--
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-rs] alamb commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894727578
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -183,6 +209,34 @@ where
Ok(b.finish())
}
+/// Extracts the quarter of a given temporal array as an array of integers
+pub fn quarter<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
Review Comment:
FWIW there was a similar PR from @ovr in DataFusion https://github.com/apache/arrow-datafusion/pull/1667/files
--
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-rs] MazterQyou commented on a diff in pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
MazterQyou commented on code in PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836#discussion_r894966737
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -183,6 +209,34 @@ where
Ok(b.finish())
}
+/// Extracts the quarter of a given temporal array as an array of integers
+pub fn quarter<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
Review Comment:
Indeed. This is regarding `date_trunc` function though, while `date_part` in DataFusion, for instance, uses `arrow-rs`'s `temporal`.
FYI I had an attempt to bring `quarter` to `date_part` in DF without adding it to `arrow-rs` first, but this required a reword of the macro and made things look weird, while having `quarter` here is a nice addition and makes the same addition in DF simple and elegant rather than hacky.
--
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-rs] tustvold merged pull request #1836: Add `quarter` support in `temporal`
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #1836:
URL: https://github.com/apache/arrow-rs/pull/1836
--
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