+1

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()

Patch updated to trunk. Compiles and tests pass.

I intend to commit in the next day or so.

Even better patch which CHANGES entry correct.

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

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.

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

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.

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).

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.

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.

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

Committed revision 1138276.