SortField.AUTO doesn't work with long

• Key: LUCENE-1045
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Grant Ingersoll
• Reporter: Daniel Naber
• Component/s: Search
• Affects Version/s: 2.2
• Fix Version/s: 2.4

Description

This is actually the same as LUCENE-463 but I cannot find a way to re-open that issue. I'm attaching a test case by dragon-fly999 at hotmail com that shows the problem and a patch that seems to fix it.

The problem is that a long (as used for dates) cannot be parsed as an integer, and the next step is then to parse it as a float, which works but which is not correct. With the patch the following parsers are used in this order: int, long, float.

Comments

[Daniel Naber]

Makes auto sorting work with long.

[Daniel Naber]

patch applied

[Grant Ingersoll]

This patch seems a bit strange to me (sorry for getting to it so late). I think the better way might be to have the FieldSortedHitQueue#comparatorAuto method use the ExtendedFieldCache, instead of creating the dependency in FieldCacheImpl on ExtendedFieldCache to have long support.

Then, you could just override getAuto in ExtendedFieldCache.

This way, those who have longs and want them treated as such get them from the ExtendedFC, otherwise, those just using FieldCacheImpl, get the behavior they are used to, namely no long support.

[Yonik Seeley]

Hmmm, I didn't realize that ExtendedFieldCache was added until you just pointed it out.
Why can't we just add long and double support directly to FieldCache?

[Grant Ingersoll]

Because it is an interface and that could break implementations. See https://issues.apache.org/jira/browse/LUCENE-1015

[Yonik Seeley]

Normally right, but a user can't provide their own implementation for lucene to use (because of the way lucene looks up the implementation).

[Grant Ingersoll]

There is a cleaner way of doing this w/o copying code from ExtendedFieldCacheImpl

Patch to follow

[Grant Ingersoll]

Normally right, but a user can't provide their own implementation for lucene to use (because of the way lucene looks up the implementation).

True, it isn't all that useful of an interface. We probably should open another issue to deal with it. Either that, or we should take another look at fixing it in https://issues.apache.org/jira/browse/LUCENE-831

[Grant Ingersoll]

Refactoring to remove duplicated code and use the ExtendedFieldCache impl.

[Yonik Seeley]

My only concern is that ExtendedFieldCache(Impl) adds more public classes that we then need to keep back compatibility with in future releases. If we decide that adding long/double directly to FieldCache is OK, then that should ideally be done before the next release so we can simply remove ExtendedFieldCache.

[Doug Cutting]

> True, it isn't all that useful of an interface.

Perhaps it should be an abstract base class instead of an interface, so that the API can more easily evolve without breaking user code? If external implementations are not currently supported, this might even be a back-compatible change.

[Grant Ingersoll]

That's fine by me, I think we just need to document it clearly in CHANGES.txt that we are making a potentially non-back compatible change (which isn't likely) in order to support future back compatibility .

I guess the question is whether there are people that have extended TopDocCollector and created a variation of the PriorityQueue that is similar to FieldSortedHitQueue that uses their own FieldCache implementation. I can't imagine the need for this, but it does seem possible, or did I miss something?

Any more thoughts on this? Otherwise, I am going to apply this patch before I forget about it. It does seem to me there is at least a remote possibility of using the interface.

I will commit tomorrow evening or Tuesday unless I hear otherwise.

[Yonik Seeley]

I think we should just fold Long & Double support into the FieldCache and change it to be an abstract class.
It is very unlikely that anyone has implemented their own FieldCache since it wouldn't be directly usable with Lucene.
In the worst-case scenario that someone did, there will be an immediate and understandable error, and the fix is easy.

[Otis Gospodnetic]

Grant, any chance of you throwing in short support in there?

[Grant Ingersoll]

short support

FieldCache already has shorts support, so no reason not to add it and bytes to SortField.

I will work up a patch for all of this.

[Grant Ingersoll]

Drops ExtendedFieldCache, puts everything into FieldCache, adds support to SortField and FieldSortedHitQueue for sorting on bytes and longs. Drops FieldCacheImpl as it doesn't really serve any purpose once you make FieldCache a class.

Note this breaks the back-compat. contract on FieldCache interface.

[Yonik Seeley]

Actually, I'm not sure we should change it to an abstract class now... that's not a backward compatible change for normal users, right?

People very likely access the current FieldCache via FieldCache.DEFAULT.get...() or
FieldCache f = FieldCache.DEFAULT

So as long as no one has any custom implementations, we can at least add new methods to the FieldCache interface and implement them in FieldCacheImpl

[Grant Ingersoll]

With this latest patch, they will still be able to do that. I made FC a full-blown public class and deleted FieldCacheImpl.

So far, there has been one user who responded to my request for people who have implemented FieldCache: http://www.gossamer-threads.com/lists/lucene/java-user/55402

However, the user already says it isn't a big deal for us to change it.

[Yonik Seeley]

> With this latest patch, they will still be able to do that.

Only if they recompile. Simply dropping in a new lucene jar would break their existing FieldCache usage.

[Grant Ingersoll]

Yes. True. Here you and Doug finally had me convinced and now...

[Grant Ingersoll]

I committed my original patch which keeps the ExtendedFieldCache.

If anything, I think we should mark FieldCache for 3.X that it is going to be converted to a class.

[Grant Ingersoll]

Sigh, I had it in the patch, but didn't get the BYTE and SHORT values on the SortField. Ugh.

[Grant Ingersoll]

Here's the patch that adds the missing short and byte sorting.

Will commit in the next day or two.

[Grant Ingersoll]

I committed the short-byte patch.