Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add before() and after() to RichDate #1538

Merged
merged 2 commits into from
Mar 23, 2016

Conversation

megaserg
Copy link
Contributor

There are two implicit conversions for RichDate:

  • implicit def toDate(rd: RichDate): java.util.Date
  • implicit def toCalendar(rd: RichDate)(implicit tz: TimeZone): java.util.Calendar

Each has its own implementation of before() and after() methods:

  • Date: boolean before(Date when)
  • Calendar: boolean before(Object when)

Their implementation in java.util is pretty much the same (comparing millis), except that Calendar's one does an additional instanceof Calendar check. Therefore, an ambiguous behavior arises:

  • Imagine a user writes: d1.before(d2), where d1, d2: RichDate.
  • Normally, implicit conversion of d1 to Date will happen, and d1.before(Date when) will be applied to d2 (converted to Date), comparing the millis.
  • However, if a user happened to have an implicit TimeZone in the context (as many Scalding job base classes do), implicit conversion of d1 to Calendar will happen, d1.before(Object when) will be applied to d2 (which is a DateRange), and the instanceof check will fail the comparison. before() will always return false!

Solution: define before() and after() methods directly in RichDate. This way, implicit conversion will not have to take place.

Complete example to reproduce. Comment/uncomment tz declaration to print true or false.

import java.util.TimeZone

import com.twitter.scalding._

object TestingImpl {
//  implicit val tz: TimeZone = TimeZone.getDefault

  val d1 = RichDate(1294239559)
  val d2 = RichDate(1295239559)

  def main(args: Array[String]) = {
    println (d1.before(d2))
  }
}

@benpence
Copy link
Contributor

@megaserg Can you add a test case that fails the instanceofcheck without these methods and succeeds with their addition?

@megaserg megaserg force-pushed the richdate-before-after-patch branch 2 times, most recently from b50ac14 to 9d10cbb Compare March 22, 2016 22:26
@@ -97,6 +98,12 @@ class DateTest extends WordSpec {
assert(rd1 >= rd1)
assert(rd2 >= rd2)
}
"be able to compare with before() and after() with TimeZone in context" in {
//implicit val tz: TimeZone = TimeZone.getDefault
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not have this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncommented. It would be failing even if I would remove the comment, but then the validness of the test would depend on that implicit tz on the top level (which someone someday might decide to remove, rendering my test trivial).

megaserg and others added 2 commits March 23, 2016 02:33
There are two implicit conversions for RichDate:
  implicit def toDate(rd: RichDate): java.util.Date
  implicit def toCalendar(rd: RichDate)(implicit tz: TimeZone): java.util.Calendar
Each has its own implementation of before() and after() methods:
  Date: boolean before(Date when)
  Calendar: boolean before(Object when)
Their implementation in java.util is pretty much the same (comparing millis), except that Calendar's one does an additional "instanceof Calendar" check. Therefore, an ambiguous behavior arises:
- Imagine a user writes: d1.before(d2), where d1, d2: RichDate.
- Normally, implicit conversion to Date will happen, and Date.before(Date when) will be applied, comparing the millis.
- However, if a user happened to have an implicit TimeZone in the context (as many Scalding job base classes do), the Calendar implicit conversion will happen, Calendar.before(Object when) will be applied, and the `instanceof` check will fail the comparison. before() will always return false!

Solution: define before() and after() methods directly in RichDate. This way, implicit conversion will not have to take place.
@megaserg megaserg force-pushed the richdate-before-after-patch branch from 9d10cbb to 615cc9e Compare March 23, 2016 09:36
@benpence
Copy link
Contributor

+1 The test fails when I run it in the console on scalding/develop

@johnynek johnynek merged commit 7380d17 into twitter:develop Mar 23, 2016
@johnynek
Copy link
Collaborator

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants