Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9080

DateMath is broken before the year 1582

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 6.0.1, 6.1
    • Component/s: None
    • Labels:
      None

      Description

      In Solr 6.0, dates are parsed using the Java 8 java.time API. It formerly was parsed using java.util.SimpleDateFormat which uses java.util.GregorianCalendar. I've learned that the java.time API does not switch to a different algorithm at the Gregorian Change Date (year 1582) whereas GregorianCalendar does. A ramification of this is that the milliseconds before epoch value is different between the APIs for dates prior to this year. They both round-trip between themselves but not between each other prior to this date. Thus, anyone indexing historical dates must re-index when moving to Solr 6.

      What was not changed in the parsing code was Solr's date-math logic – it still uses the Calendar API. This works for dates after 1582 but before, it'll introduce discrepancies. Here's an example showing weird behavior:
      http://localhost:8983/solr/techproducts/select?facet.range.end=1400-01-01T00:00:00Z&facet.range.gap=%2B10YEARS&facet.range.start=1300-01-01T00:00:00Z/YEAR&facet.range=manufacturedate_dt&facet=on&indent=on&q=*:*&rows=0&wt=json
      Note that the year 1300 rounded down to the year, becomes 1299 January 8th (weird in and of itself) and that subsequent gaps start on the 9th.

      "counts":[
                "1299-01-08T00:00:00Z",0,
                "1309-01-09T00:00:00Z",0,
                "1319-01-09T00:00:00Z",0,  ...
      

      This weirdness will show itself for units at the year or month level, but not below that (from what I'm seeing). In other words, if facet.range.gap is at this amount, or otherwise using the date math syntax to round or add a year or month, there will be issues like this. Otherwise there doesn't seem to be an issue.

      I think the solution is clearly to switch the date math code to use the java.time API.

        Issue Links

          Activity

          Hide
          dsmiley David Smiley added a comment -

          Hmmm; modifying DateMathParserTest to change most years to 1234 isn't showing an issue... need to investigate more.

          DateRangeField has some issues too that I have tests for, to be reported separately.

          Show
          dsmiley David Smiley added a comment - Hmmm; modifying DateMathParserTest to change most years to 1234 isn't showing an issue... need to investigate more. DateRangeField has some issues too that I have tests for, to be reported separately.
          Hide
          dsmiley David Smiley added a comment -

          The test said (erroneously) all was well, when I changed the year to 1234 in most test methods, for any of two reasons (or both): the test itself uses SimpleDateFormatter (which uses Calendar) as a source of truth, and because it called directly into the Calendar based utility methods of DateMathParser instead of constructing a String date math expression.

          This patch addresses both of those issues in the test, and changes most years in the test to 1234. I left some where they were because specific dates were used to craft time zone offset functionality.

          And I fixed DateMathParser itself, which was kinda fun. I removed or made non-public some things that weren't being used outside of itself or the test.

          • Note that a Locale is no longer needed/used in this API and it's dubious if it ever had an effect before, at least based on a comment about impacting when a day of the week starts (who cares?). Only the DIH DateFormatEvaluator passes something other than Locale.ROOT: it uses Locale.ENGLISH with the ability to pick something else, and it's not evident it's tested.

          This patch is probably not the final patch as I want to change the DateMathParser's API that will affect some callers.

          I'm inclined to think DateMathParser should not be something constructed – it just needs static methods. And switch away from java.util.TimeZone to java.time.ZoneId in the API. Maybe a separate issue for such things.

          Show
          dsmiley David Smiley added a comment - The test said (erroneously) all was well, when I changed the year to 1234 in most test methods, for any of two reasons (or both): the test itself uses SimpleDateFormatter (which uses Calendar) as a source of truth, and because it called directly into the Calendar based utility methods of DateMathParser instead of constructing a String date math expression. This patch addresses both of those issues in the test, and changes most years in the test to 1234. I left some where they were because specific dates were used to craft time zone offset functionality. And I fixed DateMathParser itself, which was kinda fun. I removed or made non-public some things that weren't being used outside of itself or the test. Note that a Locale is no longer needed/used in this API and it's dubious if it ever had an effect before, at least based on a comment about impacting when a day of the week starts (who cares?). Only the DIH DateFormatEvaluator passes something other than Locale.ROOT: it uses Locale.ENGLISH with the ability to pick something else, and it's not evident it's tested. This patch is probably not the final patch as I want to change the DateMathParser's API that will affect some callers. I'm inclined to think DateMathParser should not be something constructed – it just needs static methods. And switch away from java.util.TimeZone to java.time.ZoneId in the API. Maybe a separate issue for such things.
          Hide
          dsmiley David Smiley added a comment -

          This patch adds a little more testing and it modified the constructor to not take a Locale, thus affecting some callers. I removed two protected methods in DIH DateFormatEvaluator related to date processing that I don't think are necessary for subclasses to customize.

          I'll commit this Friday morning and then I'll post a follow-up issue to refine the DateMathParser API a bit to my liking.

          Show
          dsmiley David Smiley added a comment - This patch adds a little more testing and it modified the constructor to not take a Locale, thus affecting some callers. I removed two protected methods in DIH DateFormatEvaluator related to date processing that I don't think are necessary for subclasses to customize. I'll commit this Friday morning and then I'll post a follow-up issue to refine the DateMathParser API a bit to my liking.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4193e60b9fc1ff12df2267778213ae3b0f04fb84 in lucene-solr's branch refs/heads/master from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4193e60 ]

          SOLR-9080 SOLR-9085: Fix date math before the year 1582.
          note: DateMathParser no longer needs a Locale

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4193e60b9fc1ff12df2267778213ae3b0f04fb84 in lucene-solr's branch refs/heads/master from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4193e60 ] SOLR-9080 SOLR-9085 : Fix date math before the year 1582. note: DateMathParser no longer needs a Locale
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9d826ffa2f767d5e75f0844531a5c194b0c04034 in lucene-solr's branch refs/heads/branch_6x from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9d826ff ]

          SOLR-9080 SOLR-9085: Fix date math before the year 1582.
          note: DateMathParser no longer needs a Locale
          (cherry picked from commit 4193e60)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9d826ffa2f767d5e75f0844531a5c194b0c04034 in lucene-solr's branch refs/heads/branch_6x from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9d826ff ] SOLR-9080 SOLR-9085 : Fix date math before the year 1582. note: DateMathParser no longer needs a Locale (cherry picked from commit 4193e60)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8309bae5ff11c6c9e5835c60b6f8b08bd810737d in lucene-solr's branch refs/heads/branch_6_0 from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8309bae ]

          SOLR-9080 SOLR-9085: Fix date math before the year 1582.
          note: DateMathParser no longer needs a Locale
          (cherry picked from commit 4193e60)
          (cherry picked from commit 9d826ff)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8309bae5ff11c6c9e5835c60b6f8b08bd810737d in lucene-solr's branch refs/heads/branch_6_0 from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8309bae ] SOLR-9080 SOLR-9085 : Fix date math before the year 1582. note: DateMathParser no longer needs a Locale (cherry picked from commit 4193e60) (cherry picked from commit 9d826ff)
          Hide
          steve_rowe Steve Rowe added a comment -

          Bulk close issues included in the 6.0.1 release.

          Show
          steve_rowe Steve Rowe added a comment - Bulk close issues included in the 6.0.1 release.

            People

            • Assignee:
              dsmiley David Smiley
              Reporter:
              dsmiley David Smiley
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development