Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      When updating my SOLR-2533 patch, one issue was that the int value I had given my new type had been used by another change in the mean time. Since we don't use these fields in a bitset kind of way, we can convert them to an enum.

      1. LUCENE-3219.patch
        70 kB
        Chris Male
      2. LUCENE-3219.patch
        71 kB
        Chris Male
      3. LUCENE-3219.patch
        71 kB
        Chris Male
      4. LUCENE-3219.patch
        53 kB
        Chris Male

        Issue Links

          Activity

          Hide
          Simon Willnauer added a comment -

          +1

          Show
          Simon Willnauer added a comment - +1
          Hide
          Chris Male added a comment -

          First up patch which makes the majority of changes.

          I intend to commit SOLR-2533 first and then update this patch to incorporate the changes.

          Also intend to change CachedArrayCreator.getSortTypeID() to CachedArrayCreator.getSortType()

          Show
          Chris Male added a comment - First up patch which makes the majority of changes. I intend to commit SOLR-2533 first and then update this patch to incorporate the changes. Also intend to change CachedArrayCreator.getSortTypeID() to CachedArrayCreator.getSortType()
          Hide
          Chris Male added a comment -

          Patch updated to trunk. Compiles and tests pass.

          I intend to commit in the next day or so.

          Show
          Chris Male added a comment - Patch updated to trunk. Compiles and tests pass. I intend to commit in the next day or so.
          Hide
          Chris Male added a comment -

          Even better patch which CHANGES entry correct.

          Show
          Chris Male added a comment - Even better patch which CHANGES entry correct.
          Hide
          Simon Willnauer added a comment -

          chris, patch looks good...

          some minor comments:

          • I wonder if a parser could hold a Type so we could get rid of the if (parser instanceof FieldCache.$Parser) ?
          • in SearchWithSortTask I wonder if you could simply call Type.valueOf(typeString.toUpperCase()); - the less code the better

          overall looks good

          simon

          Show
          Simon Willnauer added a comment - chris, patch looks good... some minor comments: I wonder if a parser could hold a Type so we could get rid of the if (parser instanceof FieldCache.$Parser) ? in SearchWithSortTask I wonder if you could simply call Type.valueOf(typeString.toUpperCase()); - the less code the better overall looks good simon
          Hide
          Chris Male added a comment -

          Updated patch to incorporate Simon's suggestions:

          • SearchWithSortTask now uses SortField.Type.valueOf(). This changes the exception thrown to an IllegalArgumentException.
          • I haven't added Type to FieldCache.Parser since the constructor in SortField that accepts Parsers is deprecated and you can pull the Type from the CachedArrayCreator which is the preferred way of creating a SortField. I did exploit this to reduce the code in the instanceof comparisons.
          Show
          Chris Male added a comment - Updated patch to incorporate Simon's suggestions: SearchWithSortTask now uses SortField.Type.valueOf(). This changes the exception thrown to an IllegalArgumentException. I haven't added Type to FieldCache.Parser since the constructor in SortField that accepts Parsers is deprecated and you can pull the Type from the CachedArrayCreator which is the preferred way of creating a SortField. I did exploit this to reduce the code in the instanceof comparisons.
          Hide
          Simon Willnauer added a comment -

          looks good to me. BTW. should we backport those changes?

          Show
          Simon Willnauer added a comment - looks good to me. BTW. should we backport those changes?
          Hide
          Chris Male added a comment -

          You'll have to guide me on the backwards compat issue since this is a break due to the fields being public and some methods changing from returning int to returning SortField.Type.

          Show
          Chris Male added a comment - You'll have to guide me on the backwards compat issue since this is a break due to the fields being public and some methods changing from returning int to returning SortField.Type.
          Hide
          Uwe Schindler added a comment -

          This issue relates to LUCENE-3192, where I already proposed SortField.Type.

          If we want to backport to 3.x this functionality, we should add deprecated references to the SortField.Type instances directly below SortField. This would make the code not binary backwards compatible, but in most cases a simple recompile should work (most usages are new SortField("priceInCent", SortField.INT).

          Show
          Uwe Schindler added a comment - This issue relates to LUCENE-3192 , where I already proposed SortField.Type. If we want to backport to 3.x this functionality, we should add deprecated references to the SortField.Type instances directly below SortField. This would make the code not binary backwards compatible, but in most cases a simple recompile should work (most usages are new SortField("priceInCent", SortField.INT).
          Hide
          Uwe Schindler added a comment -

          At the end of the day, I am sure I will vote to leave it as it is in 3.x!

          SortField is heavy-used in Lucene client code and the backwards breaks without very sophisticated backwards layers are horrible to handle. It can be done, but I dont think its worth the work just for code beauty.

          Show
          Uwe Schindler added a comment - At the end of the day, I am sure I will vote to leave it as it is in 3.x! SortField is heavy-used in Lucene client code and the backwards breaks without very sophisticated backwards layers are horrible to handle. It can be done, but I dont think its worth the work just for code beauty.
          Hide
          Chris Male added a comment -

          For the reasons described above, I think its best we don't backport this change.

          Uwe, is the work here compatible with what you had planned in LUCENE-3192? If so, I'll go ahead and commit this.

          Show
          Chris Male added a comment - For the reasons described above, I think its best we don't backport this change. Uwe, is the work here compatible with what you had planned in LUCENE-3192 ? If so, I'll go ahead and commit this.
          Hide
          Uwe Schindler added a comment -

          Just commit this, the other issue is quite unrelated, I just had same idea.

          Show
          Uwe Schindler added a comment - Just commit this, the other issue is quite unrelated, I just had same idea.
          Hide
          Chris Male added a comment -

          Committed revision 1138276.

          Show
          Chris Male added a comment - Committed revision 1138276.

            People

            • Assignee:
              Chris Male
              Reporter:
              Chris Male
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development