Uh oh!
There was an error while loading. Please reload this page.
GH-30117: [C++][Python] Add "Z" to the end of timestamp print string when tz defined - #39272
Conversation
rok
left a comment
There was a problem hiding this comment.
Seems good. Let's just check that weird zones don't cause issues.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Rok Mihevc <rok@mihevc.org>
rok
commented
Dec 18, 2023
Forgot to ask: does this currently print timestamp's type when printing the array? And if it does can you add a test that it prints the correct timezone. If not we can leave it for the follow-up. |
AlenkaF
commented
Dec 18, 2023
No, when printing the array there is no info about the type, one should still inspect the type separately: >>>importpyarrowaspa>>>a=pa.array([0], pa.timestamp('s', tz='+02:00'))
>>>a<pyarrow.lib.TimestampArrayobjectat0x131399fc0>
[
1970-01-0100:00:00Z
]
>>>a.typeTimestampType(timestamp[s, tz=+02:00])as mentioned on the issue, we can add this if find important. |
rok
commented
Dec 18, 2023
If there's an easy way to include timezone it would be great! |
Uh oh!
There was an error while loading. Please reload this page.
jorisvandenbossche
commented
Dec 19, 2023
Given this is a completely separate code path (this is something defined in the python repr, not the C++ pretty printer), let's keep that for a separate PR? |
rok
commented
Dec 19, 2023
Agreed, let's make another issue for it. |
AlenkaF
commented
Dec 19, 2023
It would be good to add that to R also, if I am not mistaken. Will create an issue for it 👍 |
Uh oh!
There was an error while loading. Please reload this page.
AlenkaF
commented
Dec 20, 2023
Opened an issue for printing timezone information in |
jorisvandenbossche
commented
Jan 8, 2024
Thanks! |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a288364. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…tring when tz defined (apache#39272) ### What changes are included in this PR? This PR updates the PrettyPrint for Timestamp type so that "Z" is printed at the end of the output string if the timezone has been defined. This way we add minimum information about the values being stored in UTC. ### Are these changes tested? Yes. ### Are there any user-facing changes? There is a change in how `TimestampArray` prints out the data. With this change "Z" would be added to the end of the string if the timezone is defined. * Closes: apache#30117 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
…' when formatting timestamps (#41045) ### What changes are included in this PR? A test that reproduces an issue found by the fuzzer and a fix for it. ### Are these changes tested? - A test - Comments clarifying somethings around `formatting.h` - Increasing the size of the local buffer used to format timestamps The issue was introduced only recently (unreleased): #39272 * GitHub Issue: #41044 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
…the 'Z' when formatting timestamps (apache#41045) ### What changes are included in this PR? A test that reproduces an issue found by the fuzzer and a fix for it. ### Are these changes tested? - A test - Comments clarifying somethings around `formatting.h` - Increasing the size of the local buffer used to format timestamps The issue was introduced only recently (unreleased): apache#39272 * GitHub Issue: apache#41044 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
What changes are included in this PR?
This PR updates the PrettyPrint for Timestamp type so that "Z" is printed at the end of the output string if the timezone has been defined. This way we add minimum information about the values being stored in UTC.
Are these changes tested?
Yes.
Are there any user-facing changes?
There is a change in how
TimestampArrayprints out the data. With this change "Z" would be added to the end of the string if the timezone is defined.