Solr
  1. Solr
  2. SOLR-236 Field collapsing
  3. SOLR-2198

Grouping treats null values as equivalent to 0 or an empty string

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      The grouping function does not distinguish between missing values and their null type equivalents (0 for numeric types and an empty string for string types).

      1. SOLR-2198.patch
        23 kB
        Yonik Seeley
      2. SOLR-2198.patch
        13 kB
        Harish Agarwal
      3. SOLR-2198.patch
        2 kB
        Harish Agarwal

        Activity

        Hide
        Harish Agarwal added a comment -

        Attaching a patch for grouping tests which illustrates the problem.

        Show
        Harish Agarwal added a comment - Attaching a patch for grouping tests which illustrates the problem.
        Hide
        Harish Agarwal added a comment -

        A related and relevant issue is figuring out how to denote null values in the 'groupValue' field. Its still useful to group missing values together.

        Show
        Harish Agarwal added a comment - A related and relevant issue is figuring out how to denote null values in the 'groupValue' field. Its still useful to group missing values together.
        Hide
        Yonik Seeley added a comment -

        Yep, this is currently expected.

        Now that all fields can tell if there was a value or not for a given doc, we should update function query, and mutable value to reflect this.

        A related and relevant issue is figuring out how to denote null values in the 'groupValue' field. Its still useful to group missing values together.

        groupValue=null

        Show
        Yonik Seeley added a comment - Yep, this is currently expected. Now that all fields can tell if there was a value or not for a given doc, we should update function query, and mutable value to reflect this. A related and relevant issue is figuring out how to denote null values in the 'groupValue' field. Its still useful to group missing values together. groupValue=null
        Hide
        Harish Agarwal added a comment -

        I attached a patch which seems to fix the problem for fields using [Int, Long, Double, Float]FieldSource's. I made the 'exists' property on MutableValue's public and am using it to denote null values, all the non-grouping tests still pass.

        I'm still having trouble figuring out how to distinguish between a null string and "" - is this even possible?

        It would be good to get some feedback on the patch before I try to extend this to other field types (like SortableFields).

        Show
        Harish Agarwal added a comment - I attached a patch which seems to fix the problem for fields using [Int, Long, Double, Float] FieldSource's. I made the 'exists' property on MutableValue's public and am using it to denote null values, all the non-grouping tests still pass. I'm still having trouble figuring out how to distinguish between a null string and "" - is this even possible? It would be good to get some feedback on the patch before I try to extend this to other field types (like SortableFields).
        Hide
        Harish Agarwal added a comment -

        Wanted to check in on this issue, have any commiters had a chance to look at the patch? What do I need to do to move this forward? Thanks!

        Show
        Harish Agarwal added a comment - Wanted to check in on this issue, have any commiters had a chance to look at the patch? What do I need to do to move this forward? Thanks!
        Hide
        Yonik Seeley added a comment -

        Here's an updated patch based on Harish's patch, with support for additional field types, with some additional changes.

        • always set the value, regardless of the value of exists - this simplifies hashCode
        • move the comparison of "exists" into subclasses so equalsSameTypecan be directly used by comparators
        • add support in compareTo

        This is untested - I don't know if it works yet. There are other issues with random testing and sorting null values that I need to iron out first.

        Show
        Yonik Seeley added a comment - Here's an updated patch based on Harish's patch, with support for additional field types, with some additional changes. always set the value, regardless of the value of exists - this simplifies hashCode move the comparison of "exists" into subclasses so equalsSameTypecan be directly used by comparators add support in compareTo This is untested - I don't know if it works yet. There are other issues with random testing and sorting null values that I need to iron out first.
        Hide
        Yonik Seeley added a comment -

        OK, I added random testing support and committed.

        Show
        Yonik Seeley added a comment - OK, I added random testing support and committed.

          People

          • Assignee:
            Unassigned
            Reporter:
            Harish Agarwal
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development