Add quarter support in temporal
#1836
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1836 +/- ##
=======================================
Coverage 83.48% 83.48%
=======================================
Files 201 201
Lines 56838 56878 +40
=======================================
+ Hits 47451 47485 +34
- Misses 9387 9393 +6
Continue to review full report at Codecov.
|
| #[test] | ||
| fn test_temporal_array_date64_quarter() { | ||
| //1514764800000 -> 2018-01-01 | ||
| //1566275025000 -> 2019-08-20 |
There was a problem hiding this comment.
Is it worth checking a month on the leading edge, e.g. first day of April
There was a problem hiding this 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.
| trait ChronoDateQuarter { | ||
| fn quarter(&self) -> u32; | ||
|
|
||
| fn quarter0(&self) -> u32; |
There was a problem hiding this comment.
Is there a particular reason to have quarter0?
There was a problem hiding this 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.
| @@ -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> | |||
There was a problem hiding this comment.
FWIW there was a similar PR from @ovr in DataFusion https://github.com/apache/arrow-datafusion/pull/1667/files
There was a problem hiding this 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.
MazterQyou
force-pushed
the
upstream/temporal-quarter
branch
from
d905c47 to
1711167
Compare
MazterQyou
force-pushed
the
upstream/temporal-quarter
branch
from
1711167 to
a7223bb
Compare
Can drop this after rebase on commit 36ceb22 "Add quarter support in temporal (apache#1836)", first released in 17.0.0
Can drop this after rebase on commit 36ceb22 "Add quarter support in temporal (apache#1836)", first released in 17.0.0
Can drop this after rebase on commit 36ceb22 "Add quarter support in temporal (apache#1836)", first released in 17.0.0
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_partSQL builtin to supportquarterfield extraction.What changes are included in this PR?
This PR adds a
quarterextractor totemporal, and a few related tests.Are there any user-facing changes?