Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.4
    • Component/s: core/search
    • Labels:
      None

      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.

      1. auto-long-sorting.diff
        4 kB
        Daniel Naber
      2. LUCENE-1045.patch
        67 kB
        Grant Ingersoll
      3. LUCENE-1045.patch
        7 kB
        Grant Ingersoll
      4. LUCENE-1045-shortbyte.patch
        11 kB
        Grant Ingersoll
      5. TestDateSort.java
        4 kB
        Daniel Naber

        Issue Links

          Activity

          Hide
          lucenebugs@danielnaber.de Daniel Naber added a comment -

          Makes auto sorting work with long.

          Show
          lucenebugs@danielnaber.de Daniel Naber added a comment - Makes auto sorting work with long.
          Hide
          lucenebugs@danielnaber.de Daniel Naber added a comment -

          patch applied

          Show
          lucenebugs@danielnaber.de Daniel Naber added a comment - patch applied
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          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?

          Show
          yseeley@gmail.com Yonik Seeley added a comment - 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?
          Hide
          gsingers Grant Ingersoll added a comment -

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

          Show
          gsingers Grant Ingersoll added a comment - Because it is an interface and that could break implementations. See https://issues.apache.org/jira/browse/LUCENE-1015
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

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

          Show
          yseeley@gmail.com Yonik Seeley added a comment - Normally right, but a user can't provide their own implementation for lucene to use (because of the way lucene looks up the implementation).
          Hide
          gsingers Grant Ingersoll added a comment -

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

          Patch to follow

          Show
          gsingers Grant Ingersoll added a comment - There is a cleaner way of doing this w/o copying code from ExtendedFieldCacheImpl Patch to follow
          Hide
          gsingers Grant Ingersoll added a comment -

          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

          Show
          gsingers Grant Ingersoll added a comment - 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
          Hide
          gsingers Grant Ingersoll added a comment -

          Refactoring to remove duplicated code and use the ExtendedFieldCache impl.

          Show
          gsingers Grant Ingersoll added a comment - Refactoring to remove duplicated code and use the ExtendedFieldCache impl.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          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.

          Show
          yseeley@gmail.com Yonik Seeley added a comment - 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.
          Hide
          cutting Doug Cutting added a comment -

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

          Show
          cutting Doug Cutting added a comment - > 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.
          Hide
          gsingers Grant Ingersoll added a comment -

          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?

          Show
          gsingers Grant Ingersoll added a comment - 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?
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          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.

          Show
          yseeley@gmail.com Yonik Seeley added a comment - 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.
          Hide
          otis Otis Gospodnetic added a comment -

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

          Show
          otis Otis Gospodnetic added a comment - Grant, any chance of you throwing in short support in there?
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

          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

          Show
          yseeley@gmail.com Yonik Seeley added a comment - 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
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

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

          Show
          yseeley@gmail.com Yonik Seeley added a comment - > 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.
          Hide
          gsingers Grant Ingersoll added a comment -

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

          Show
          gsingers Grant Ingersoll added a comment - Yes. True. Here you and Doug finally had me convinced and now...
          Hide
          gsingers Grant Ingersoll added a comment -

          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.

          Show
          gsingers Grant Ingersoll added a comment - 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.
          Hide
          gsingers Grant Ingersoll added a comment -

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

          Show
          gsingers Grant Ingersoll added a comment - Sigh, I had it in the patch, but didn't get the BYTE and SHORT values on the SortField. Ugh.
          Hide
          gsingers Grant Ingersoll added a comment -

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

          Will commit in the next day or two.

          Show
          gsingers Grant Ingersoll added a comment - Here's the patch that adds the missing short and byte sorting. Will commit in the next day or two.
          Hide
          gsingers Grant Ingersoll added a comment -

          I committed the short-byte patch.

          Show
          gsingers Grant Ingersoll added a comment - I committed the short-byte patch.

            People

            • Assignee:
              gsingers Grant Ingersoll
              Reporter:
              lucenebugs@danielnaber.de Daniel Naber
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development