Lucene - Core
  1. Lucene - Core
  2. LUCENE-500

Lucene 2.0 requirements - Remove all deprecated code

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 2.0.0
    • Component/s: None
    • Labels:
      None

      Description

      Per the move to Lucene 2.0 from 1.9, remove all deprecated code and update documentation, etc.

      Patch to follow shortly.

      1. deprecation.txt
        390 kB
        Grant Ingersoll
      2. deprecation2.txt
        387 kB
        Grant Ingersoll

        Activity

        Hide
        Grant Ingersoll added a comment -

        Removed all deprecated methods/classes/tests/files. I know the paint isn't dry on 1.9, but thought I would get a leap on 2.0...

        Other notes:

        1. Analyzer.java – Made the tokenStream(String, Reader) into an abstract method.

        2. Some items that were marked as deprecated actually needed a reduction in visibility from public to private, and the getter/setters referenced instead

        3. All tests pass.

        4. Removed test-deprecated.

        5. The file deprecation-notes.txt are most likely included in the patch, but they can be deleted, as this comment is more recent.

        6. I did not review the documentation (i.e. javadocs, demo/tutorial, etc.) to look for references to deprecated items

        Let me know if there is anything else that is needed.

        -Grant

        Show
        Grant Ingersoll added a comment - Removed all deprecated methods/classes/tests/files. I know the paint isn't dry on 1.9, but thought I would get a leap on 2.0... Other notes: 1. Analyzer.java – Made the tokenStream(String, Reader) into an abstract method. 2. Some items that were marked as deprecated actually needed a reduction in visibility from public to private, and the getter/setters referenced instead 3. All tests pass. 4. Removed test-deprecated. 5. The file deprecation-notes.txt are most likely included in the patch, but they can be deleted, as this comment is more recent. 6. I did not review the documentation (i.e. javadocs, demo/tutorial, etc.) to look for references to deprecated items Let me know if there is anything else that is needed. -Grant
        Hide
        Doug Cutting added a comment -

        +1

        This looks like a good start towards 2.0.

        Show
        Doug Cutting added a comment - +1 This looks like a good start towards 2.0.
        Hide
        Daniel Naber added a comment -

        DateField should not be removed, as that would mean people cannot easily access date values created with Lucene 1.4. I know the fact that it's deprecated is strange, but I didn't know how else to draw people's attention to the fact that they should (not must) use DateTools.

        Show
        Daniel Naber added a comment - DateField should not be removed, as that would mean people cannot easily access date values created with Lucene 1.4. I know the fact that it's deprecated is strange, but I didn't know how else to draw people's attention to the fact that they should (not must) use DateTools.
        Hide
        Grant Ingersoll added a comment -

        Does that mean, then, that the usages of it in the QueryParser and the DateFilter need to be preserved as well? Doesn't that mean that anyone using the QP to do range queries will need their index to be created using DateField instead of DateTools?

        Is there a way we can make DateTools be backward compatible to DateField when appropriate? So that we could still remove DateField?

        Show
        Grant Ingersoll added a comment - Does that mean, then, that the usages of it in the QueryParser and the DateFilter need to be preserved as well? Doesn't that mean that anyone using the QP to do range queries will need their index to be created using DateField instead of DateTools? Is there a way we can make DateTools be backward compatible to DateField when appropriate? So that we could still remove DateField?
        Hide
        Daniel Naber added a comment -

        Answer to the first questions: yes and yes. DateTools could be made to read the old values transparently. Also the method to create the old format strings could be moved to DateTools, but the comment in DateField actually says that the class will not be removed.

        Show
        Daniel Naber added a comment - Answer to the first questions: yes and yes. DateTools could be made to read the old values transparently. Also the method to create the old format strings could be moved to DateTools, but the comment in DateField actually says that the class will not be removed.
        Hide
        Grant Ingersoll added a comment -

        OK, but then what is the plan for generating Range Queries in the QueryParser for those indexes that use DateTools instead? Or is it not an issue?

        Show
        Grant Ingersoll added a comment - OK, but then what is the plan for generating Range Queries in the QueryParser for those indexes that use DateTools instead? Or is it not an issue?
        Hide
        Daniel Naber added a comment -

        I have no plan to fix this, because I consider this a rather exotic feature and because it should not be difficult to extend QueryParser so that it supports the new format. If you know an elegant solution, patches are welcome.

        Show
        Daniel Naber added a comment - I have no plan to fix this, because I consider this a rather exotic feature and because it should not be difficult to extend QueryParser so that it supports the new format. If you know an elegant solution, patches are welcome.
        Hide
        Grant Ingersoll added a comment -

        I am not sure what the implications of not changing it are. Are Query Parser generated Range Queries really that exotic?

        I am more than happy to roll back the involved code, but I am not sure if that is the right thing to do. I know we have a large install base, but it seems to me that people understand that a major revision (i.e moving from 1.x to 2.x) is going to result in some things breaking and that, if they want to upgrade to 2.0, then they will have to do some more work.

        At any rate, I will take another look at the code involved and see if I have any bright ideas for how to deal with it.

        Show
        Grant Ingersoll added a comment - I am not sure what the implications of not changing it are. Are Query Parser generated Range Queries really that exotic? I am more than happy to roll back the involved code, but I am not sure if that is the right thing to do. I know we have a large install base, but it seems to me that people understand that a major revision (i.e moving from 1.x to 2.x) is going to result in some things breaking and that, if they want to upgrade to 2.0, then they will have to do some more work. At any rate, I will take another look at the code involved and see if I have any bright ideas for how to deal with it.
        Hide
        Daniel Naber added a comment -

        RangeQueries built via QueryParser are probably common, but not necessarily those that parse the local date format to a DateField, and only these are affected. I'm not sure what to do either, but we can read old index files so we somehow need to make sure the data in the old format can still be interpreted.

        Show
        Daniel Naber added a comment - RangeQueries built via QueryParser are probably common, but not necessarily those that parse the local date format to a DateField, and only these are affected. I'm not sure what to do either, but we can read old index files so we somehow need to make sure the data in the old format can still be interpreted.
        Hide
        Grant Ingersoll added a comment -

        What can I do to move this forward?

        Show
        Grant Ingersoll added a comment - What can I do to move this forward?
        Hide
        Yonik Seeley added a comment -

        My take is that QueryParser should have never attempted to guess the type of the field.
        It creates errors when you query on something that looks like a date, but isn't indexed using DateField or DateTools.

        I guess we still need to make a choice on whether breaking the QueryParser compatability is OK or not for 2.0

        Show
        Yonik Seeley added a comment - My take is that QueryParser should have never attempted to guess the type of the field. It creates errors when you query on something that looks like a date, but isn't indexed using DateField or DateTools. I guess we still need to make a choice on whether breaking the QueryParser compatability is OK or not for 2.0
        Hide
        Doug Cutting added a comment -

        I think we should commit this, except leave DateField deprecated rather than remove it, since that is the promise made. We should probably remove that promise, however, so that we can eventually remove it.

        We said we'd remove all deprecated code, and we won't. But that won't break anyone who compiles in 1.9.1 without deprecation warnings, which is the more important promise.

        Then we'll have a proto-2.0 release for developers to test. We can separately decide how to handle dates in the QueryParser. It would not be wrong to continue doing the same thing we've done in 1.9, would it? That won't break anyone.

        In short, let's not let this issue stop us from removing all of the other deprecated code and finding out whether anything else breaks.

        Show
        Doug Cutting added a comment - I think we should commit this, except leave DateField deprecated rather than remove it, since that is the promise made. We should probably remove that promise, however, so that we can eventually remove it. We said we'd remove all deprecated code, and we won't. But that won't break anyone who compiles in 1.9.1 without deprecation warnings, which is the more important promise. Then we'll have a proto-2.0 release for developers to test. We can separately decide how to handle dates in the QueryParser. It would not be wrong to continue doing the same thing we've done in 1.9, would it? That won't break anyone. In short, let's not let this issue stop us from removing all of the other deprecated code and finding out whether anything else breaks.
        Hide
        Otis Gospodnetic added a comment -

        Grant, it looks like we have a good plan for going forward.
        Can you/do you want to submit a new patch, or should I try to modify the original one?

        Show
        Otis Gospodnetic added a comment - Grant, it looks like we have a good plan for going forward. Can you/do you want to submit a new patch, or should I try to modify the original one?
        Hide
        Grant Ingersoll added a comment -

        OK, here is the update. I restored DateField (so it is not removed in the patch) and it's usage in QueryParser.

        I did not restore it's usage anywhere else outside of QP and it's Test.

        All tests pass.

        Show
        Grant Ingersoll added a comment - OK, here is the update. I restored DateField (so it is not removed in the patch) and it's usage in QueryParser. I did not restore it's usage anywhere else outside of QP and it's Test. All tests pass.
        Hide
        Yonik Seeley added a comment -

        Patch applied, thanks Grant!

        Note: for some reason, I couldn't get the patch to apply cleanly to files being removed. I removed those by hand.

        Show
        Yonik Seeley added a comment - Patch applied, thanks Grant! Note: for some reason, I couldn't get the patch to apply cleanly to files being removed. I removed those by hand.
        Hide
        Yonik Seeley added a comment -

        Looks like there are still some things in contrib that use deprecated stuff... I'll look into it.

        Show
        Yonik Seeley added a comment - Looks like there are still some things in contrib that use deprecated stuff... I'll look into it.
        Hide
        Yonik Seeley added a comment -

        OK, I think I got all of the deprecation related failures in "ant test-contrib".
        It's tough to tell for sure since there are many other types of failures in test-contrib.

        Show
        Yonik Seeley added a comment - OK, I think I got all of the deprecation related failures in "ant test-contrib". It's tough to tell for sure since there are many other types of failures in test-contrib.
        Hide
        Daniel Naber added a comment -

        Did files like DateFilter really get removed? On my local checkout DateFilter.java is now an empty file. ViewCVS on the web also still shows the file. Or is that expected behavior with subversion?

        Show
        Daniel Naber added a comment - Did files like DateFilter really get removed? On my local checkout DateFilter.java is now an empty file. ViewCVS on the web also still shows the file. Or is that expected behavior with subversion?
        Hide
        Yonik Seeley added a comment -

        OK, I removed the zero length files.
        Should should the empty src/test-deprecated directory hierarchy also be removed, or should it be left in place when other tests become deprecated?

        Show
        Yonik Seeley added a comment - OK, I removed the zero length files. Should should the empty src/test-deprecated directory hierarchy also be removed, or should it be left in place when other tests become deprecated?
        Hide
        Doug Cutting added a comment -

        +1 for removing the empty directory. It can always be recreated if it is needed again.

        Show
        Doug Cutting added a comment - +1 for removing the empty directory. It can always be recreated if it is needed again.
        Hide
        Daniel Naber added a comment -

        It seems QueryParser.java has been patched, but not QueryParser.jj.

        Show
        Daniel Naber added a comment - It seems QueryParser.java has been patched, but not QueryParser.jj.
        Hide
        Yonik Seeley added a comment -

        Good catch Daniel... I'll try and get to it later tonight.

        Show
        Yonik Seeley added a comment - Good catch Daniel... I'll try and get to it later tonight.
        Hide
        Paul Elschot added a comment -

        Here, at revision 387786, the target common.compile-test fails because a few previously deprecated methods
        are still being used in test code (Field creation and BooleanQuery.add() in TestKipping* and TestRewriting).
        Is there someone looking into this, or shall I provide a patch?

        Show
        Paul Elschot added a comment - Here, at revision 387786, the target common.compile-test fails because a few previously deprecated methods are still being used in test code (Field creation and BooleanQuery.add() in TestKipping* and TestRewriting). Is there someone looking into this, or shall I provide a patch?
        Hide
        Paul Elschot added a comment -

        Oops, I checked svn status for the test code, and the two tests that cause these compiler errors
        never made it into the trunk, so please ignore this.

        Show
        Paul Elschot added a comment - Oops, I checked svn status for the test code, and the two tests that cause these compiler errors never made it into the trunk, so please ignore this.
        Hide
        Yonik Seeley added a comment -

        Closing... I think we pretty much covered this.

        Show
        Yonik Seeley added a comment - Closing... I think we pretty much covered this.

          People

          • Assignee:
            Yonik Seeley
            Reporter:
            Grant Ingersoll
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development