GH-30036: [C++] Timezone-aware kernels should handle offset strings (e.g. "+04:30")#12865
GH-30036: [C++] Timezone-aware kernels should handle offset strings (e.g. "+04:30")#12865rok merged 23 commits intoapache:mainfrom
Conversation
|
@pitrou @lidavidm @jorisvandenbossche this is complete yet but I would like to validate the approach with you before I continue. Do you feel this makes enough sense to complete the PR? |
What is this comment in reference to? It seems this PR would handle 30 minute iterations correctly? |
Current implementation can handle fixed offsets of iterations of one hour. This PR would enable offsets of arbitrary number of minutes. So primary use of that would indeed be to operate in |
ead6a33 to
b7c3038
Compare
0f1f432 to
53ec435
Compare
| namespace compute { | ||
| namespace internal { | ||
|
|
||
| #include "arrow/compute/kernels/codegen_internal.h" |
There was a problem hiding this comment.
What is codegen_internal.h for here?
There was a problem hiding this comment.
It's to get arrow::compute::internal::applicator::ScalarUnaryNotNullStateful and arrow::compute::match::TimestampTypeUnit.
| std::chrono::minutes zone_offset; | ||
| switch (timezone.length()) { | ||
| case 6: | ||
| if (arrow::internal::detail::ParseHH_MM(offset.c_str(), &zone_offset)) { |
There was a problem hiding this comment.
We should probably update this to operate on string_view but that can be done later
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
|
Thank you for the review @lidavidm ! |
|
@github-actions crossbow submit -g cpp |
|
Revision: 936d020 Submitted crossbow builds: ursacomputing/crossbow @ actions-744a53bba9
|
|
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit bf342b2. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
ARROW-14477: #30036
Currently timestamp arrays have unit
timestamp(unit, zone name). This would add "offset timezones" where timestamp array would also support units liketimestamp(unit, "+/-HH:MM").