Lucene - Core
  1. Lucene - Core
  2. LUCENE-6719

NumericUtils.getMinLong and NumericUtils.getMaxLong have undefined behavior when no docs have value - throw NPE

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Tracked down a possible cause of SOLR-7866 to situations where a (numeric) field doesn't have any values in an index and you try to get the min/max.

      javadocs for NumericUtils.getMinLong and NumericUtils.getMaxLong don't actually say what that method will do in this case, throw NPE when it happens

      1. LUCENE-6719.patch
        6 kB
        Timothy Potter
      2. LUCENE-6719.patch
        5 kB
        Timothy Potter
      3. LUCENE-6719.patch
        6 kB
        Hoss Man
      4. LUCENE-6719.patch
        1 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          patch with test demonstrating problem

          Show
          Hoss Man added a comment - patch with test demonstrating problem
          Hide
          Hoss Man added a comment -

          updated patch with:

          • strawman solution:
            • document that the existing methods throw NPE
            • deprecate the existing methods
            • new methods that return Integer and Long and are documented to return null when appropriate
          • much better tests - both of the old methods and the new methods
          Show
          Hoss Man added a comment - updated patch with: strawman solution: document that the existing methods throw NPE deprecate the existing methods new methods that return Integer and Long and are documented to return null when appropriate much better tests - both of the old methods and the new methods
          Hide
          Michael McCandless added a comment -

          Thanks for tracking this down Hoss Man.

          Instead of adding new methods can you just fix the current ones? I think it's weird to have getMinLong and getMinimumLong, and I think it's OK to upgrade from int to Integer return value.

          Show
          Michael McCandless added a comment - Thanks for tracking this down Hoss Man . Instead of adding new methods can you just fix the current ones? I think it's weird to have getMinLong and getMinimumLong , and I think it's OK to upgrade from int to Integer return value.
          Hide
          Hoss Man added a comment -

          I think it's OK to upgrade from int to Integer return value.

          Hmmm ... depends on your definition of "OK" ... changing the return type definitely breaks backcompat on the method sig.

          hossman@tray:~/tmp$ cat Foo.java 
          public class Foo {
              public static int foo() { return 42; }
              // public static Integer foo() { return 42; }
          }
          hossman@tray:~/tmp$ cat Bar.java 
          public class Bar {
              public static void main(String[] args) {
                  int i = Foo.foo();
                  System.out.println(i);
              }
          }
          hossman@tray:~/tmp$ javac Foo.java 
          hossman@tray:~/tmp$ javac Bar.java 
          hossman@tray:~/tmp$ java Bar 
          42
          hossman@tray:~/tmp$ echo "edit Foo"
          edit Foo
          hossman@tray:~/tmp$ cat Foo.java 
          public class Foo {
              // public static int foo() { return 42; }
              public static Integer foo() { return 42; }
          }
          hossman@tray:~/tmp$ javac Foo.java 
          hossman@tray:~/tmp$ java Bar 
          Exception in thread "main" java.lang.NoSuchMethodError: Foo.foo()I
          	at Bar.main(Bar.java:3)
          
          Show
          Hoss Man added a comment - I think it's OK to upgrade from int to Integer return value. Hmmm ... depends on your definition of "OK" ... changing the return type definitely breaks backcompat on the method sig. hossman@tray:~/tmp$ cat Foo.java public class Foo { public static int foo() { return 42; } // public static Integer foo() { return 42; } } hossman@tray:~/tmp$ cat Bar.java public class Bar { public static void main(String[] args) { int i = Foo.foo(); System.out.println(i); } } hossman@tray:~/tmp$ javac Foo.java hossman@tray:~/tmp$ javac Bar.java hossman@tray:~/tmp$ java Bar 42 hossman@tray:~/tmp$ echo "edit Foo" edit Foo hossman@tray:~/tmp$ cat Foo.java public class Foo { // public static int foo() { return 42; } public static Integer foo() { return 42; } } hossman@tray:~/tmp$ javac Foo.java hossman@tray:~/tmp$ java Bar Exception in thread "main" java.lang.NoSuchMethodError: Foo.foo()I at Bar.main(Bar.java:3)
          Hide
          Adrien Grand added a comment -

          I don't think we ever tried to keep binary compability except for bugfix releases, so I don't see it as an issue: users upgrading to 5.3 will just have to recompile their application. Moreover NumericUtils is documented as an internal class so we are free to break compatibility.

          Show
          Adrien Grand added a comment - I don't think we ever tried to keep binary compability except for bugfix releases, so I don't see it as an issue: users upgrading to 5.3 will just have to recompile their application. Moreover NumericUtils is documented as an internal class so we are free to break compatibility.
          Hide
          Timothy Potter added a comment -

          Hossman is OOO ... so I'm taking this up as I'd like to get SOLR-7866 into 5.3.

          Show
          Timothy Potter added a comment - Hossman is OOO ... so I'm taking this up as I'd like to get SOLR-7866 into 5.3.
          Hide
          Michael McCandless added a comment -

          +1, thanks Timothy Potter

          Show
          Michael McCandless added a comment - +1, thanks Timothy Potter
          Hide
          Timothy Potter added a comment -

          Here's the updated patch with the lucene/CHANGES.txt entry ... I'm ready to commit this.

          Show
          Timothy Potter added a comment - Here's the updated patch with the lucene/CHANGES.txt entry ... I'm ready to commit this.
          Hide
          ASF subversion and git services added a comment -

          Commit 1694384 from Timothy Potter in branch 'dev/trunk'
          [ https://svn.apache.org/r1694384 ]

          LUCENE-6719: Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field

          Show
          ASF subversion and git services added a comment - Commit 1694384 from Timothy Potter in branch 'dev/trunk' [ https://svn.apache.org/r1694384 ] LUCENE-6719 : Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field
          Hide
          ASF subversion and git services added a comment -

          Commit 1694386 from Timothy Potter in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1694386 ]

          LUCENE-6719: Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field

          Show
          ASF subversion and git services added a comment - Commit 1694386 from Timothy Potter in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694386 ] LUCENE-6719 : Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field
          Hide
          ASF subversion and git services added a comment -

          Commit 1694387 from Timothy Potter in branch 'dev/branches/lucene_solr_5_3'
          [ https://svn.apache.org/r1694387 ]

          LUCENE-6719: Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field

          Show
          ASF subversion and git services added a comment - Commit 1694387 from Timothy Potter in branch 'dev/branches/lucene_solr_5_3' [ https://svn.apache.org/r1694387 ] LUCENE-6719 : Fix NumericUtils getMin/Max methods to return null if there are no terms for the specified field
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

            People

            • Assignee:
              Timothy Potter
              Reporter:
              Hoss Man
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development