ARROW-12011: [C++] Fix crashes and incorrect results when printing extreme date values #10988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pitrou
force-pushed
the
ARROW-12011-date-formatting
branch
from
59361cf
to
4e6020b
Compare
|
@jorisvandenbossche Would you like to review some C++ code? |
bkietz
requested changes
Aug 30, 2021
cpp/src/arrow/pretty_print.cc
Outdated
| [&](int64_t i) { FormatDateTime(type->unit(), "%T", data[i], false); }); | ||
| WriteValues(array, [&](int64_t i) { | ||
| const int32_t v = data[i]; | ||
| if (v >= -12687428 && v <= 11248737) { |
There was a problem hiding this comment.
Nit: please extract named constants like
Suggested change
| if (v >= -12687428 && v <= 11248737) { | |
| if (v >= kMinimumDay && v <= kMaximumDay) { |
cpp/src/arrow/pretty_print.cc
Outdated
| (*sink_) << arrow_vendored::date::format(fmt, Unit{value}); | ||
| try { | ||
| if (add_epoch) { | ||
| (*sink_) << arrow_vendored::date::format(fmt, epoch_ + Unit{value}); |
There was a problem hiding this comment.
The range check could possibly even just be here:
Suggested change
| (*sink_) << arrow_vendored::date::format(fmt, epoch_ + Unit{value}); | |
| constexpr Unit kMin = duration_cast<Unit>(kMinDay); | |
| constexpr Unit kMax = duration_cast<Unit>(kMaxDay); | |
| if (Unit{value} >= kMin && Unit{value} <= kMax) { | |
| (*sink_) << arrow_vendored::date::format(fmt, epoch_ + Unit{value}); | |
| } else { | |
| WriteOutOfRange(value); | |
| } |
There was a problem hiding this comment.
Hmm... nice idea, but it unfortunately overflows for nanoseconds.
/home/antoine/arrow/dev/cpp/src/arrow/pretty_print.cc: In instantiation of 'void arrow::{anonymous}::ArrayPrinter::FormatDateTime(const char*, int64_t, bool) [with Unit = std::chrono::duration<long int, std::ratio<1, 1000000000> >; int64_t = long int]':
/home/antoine/arrow/dev/cpp/src/arrow/pretty_print.cc:505:71: required from here
/home/antoine/arrow/dev/cpp/src/arrow/pretty_print.cc:483:59: in 'constexpr' expansion of 'std::chrono::duration_cast<std::chrono::duration<long int, std::ratio<1, 1000000000> >, int, std::ratio<86400, 1> >(std::chrono::duration<int, std::ratio<86400, 1> >(-12687428))'
/usr/include/c++/9/chrono:200:21: in 'constexpr' expansion of 'std::chrono::__duration_cast_impl<std::chrono::duration<long int, std::ratio<1, 1000000000> >, std::ratio<86400000000000, 1>, long int, false, true>::__cast<int, std::ratio<86400, 1> >((* & __d))'
/home/antoine/arrow/dev/cpp/src/arrow/pretty_print.cc:483:20: error: overflow in constant expression [-fpermissive]
483 | constexpr Unit kMin = std::chrono::duration_cast<Unit>(arrow_vendored::date::days{-12687428});
| ^~~~
There was a problem hiding this comment.
Ok, it works by templating a bit differently.
pitrou
force-pushed
the
ARROW-12011-date-formatting
branch
from
4e6020b
to
3917ed1
Compare
|
I didn't yet look at the code, but one note: the That can certainly be for another JIRA, but wondering a bit if it might be possible to share some of those checks (the printing should also work if compute is not available I suppose, so cannot directly rely on it) |
…treme date values The `arrow_vendored::date` library represents year numbers as a C short and may silently wraparound its value (but also throw an exception if the year has the value -32768).
pitrou
force-pushed
the
ARROW-12011-date-formatting
branch
from
f7b8f97
to
4c6b981
Compare
|
I will merge this, feel free to post any comments afterwards, though. |
zeroshade
pushed a commit
to zeroshade/arrow
that referenced
this pull request
Sep 12, 2021
…treme date values The `arrow_vendored::date` library represents year numbers as a C short and may silently wraparound its value (but also throw an exception if the year has the value -32768). Closes apache#10988 from pitrou/ARROW-12011-date-formatting Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
ViniciusSouzaRoque
pushed a commit
to s1mbi0se/arrow
that referenced
this pull request
Oct 20, 2021
…treme date values The `arrow_vendored::date` library represents year numbers as a C short and may silently wraparound its value (but also throw an exception if the year has the value -32768). Closes apache#10988 from pitrou/ARROW-12011-date-formatting Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
arrow_vendored::datelibrary represents year numbers as a C shortand may silently wraparound its value
(but also throw an exception if the year has the value -32768).