Fix Days.daysBetween int overflow #471
Conversation
TuanNguyen27
requested review from
gerashegalov,
Jauntbox,
leahmcguire,
tovbinm and
wsuchy
as code owners
| @@ -122,7 +122,7 @@ class DateMapVectorizerTest extends FlatSpec with TestSparkContext with Attribut | |||
| } | |||
|
|
|||
| it should "vectorize dates correctly on test date" in { | |||
| checkAt(new JDateTime(2017, 9, 28, 15, 45, 39, DateTimeUtils.DefaultTimeZone)) | |||
| checkAt(new JDateTime(1901, 1, 1, 0, 0, 0, DateTimeUtils.DefaultTimeZone)) | |||
There was a problem hiding this comment.
better add a new test case
There was a problem hiding this comment.
@TuanNguyen27 please add test cases for ArithmeticExceptions you are trying to solve
There was a problem hiding this comment.
@gerashegalov the new test case is checkAt(new JDateTime(1901, 1, 1, 0, 0, 0, DateTimeUtils.DefaultTimeZone)), which exceed the limit if we use Days.daysBetween. Does that work ?
| @@ -397,7 +397,7 @@ final class DateMapVectorizerModel[T <: OPMap[Long]] private[op] | |||
| val timeZone: DateTimeZone = DateTimeUtils.DefaultTimeZone | |||
|
|
|||
| def convertFn: DateMap#Value => RealMap#Value = (dt: DateMap#Value) => | |||
| dt.mapValues(v => Days.daysBetween(new DateTime(v, timeZone), referenceDate).getDays.toDouble) | |||
| dt.mapValues(v => new Duration(new DateTime(v, timeZone), referenceDate).getStandardDays.toDouble) | |||
There was a problem hiding this comment.
also, there are more places like this in the code. try grepping.
There was a problem hiding this comment.
there is at least one more in DateTimeVectorizerTest
|
please add a real PR description |
TuanNguyen27
changed the title
Codecov Report
@@ Coverage Diff @@
## master #471 +/- ##
=========================================
Coverage ? 86.99%
=========================================
Files ? 345
Lines ? 11622
Branches ? 609
=========================================
Hits ? 10110
Misses ? 1512
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM,
@TuanNguyen27 I refactored the code to avoid code duplication
(cherry picked from commit dc33ada)
(cherry picked from commit dc33ada) Co-authored-by: Tuan Nguyen <[email protected]>
Days.daysBetween(datetime_1, datetime_2)could throwjava.lang.ArithmeticException: Value cannot fit in an int:if the duration between two dates in milliseconds cannot fit a 32-bit int representation. See related Stack Overflow issue hereDescribe the proposed solution
Switch to using joda's
Duration.Describe alternatives you've considered
N/A
Additional context
N/A