Lucene - Core
  1. Lucene - Core
  2. LUCENE-1808

make Query.createWeight public (or add back Query.createQueryWeight())

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: core/query/scoring
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Now that the QueryWeight class has been removed, the public QueryWeight createQueryWeight() method on Query was also removed

      i have cases where i want to create a weight for a sub query (outside of the org.apache.lucene.search package) and i don't want the weight normalized (think BooleanQuery outside of the o.a.l.search package)

      in order to do this, i have to create a static Utils class inside o.a.l.search, pass in the Query and searcher, and have the static method call the protected createWeight method
      this should not be necessary

      This could be fixed in one of 2 ways:
      1. make createWeight() public on Query (breaks back compat)
      2. add the following method:

      public Weight createQueryWeight(Searcher searcher) throws IOException {
        return createWeight(searcher);
      }
      

      createWeight(Searcher) should then be deprectated in favor of the publicly accessible method

        Activity

        Hide
        Mark Miller added a comment -

        Thanks all!

        r805189

        Show
        Mark Miller added a comment - Thanks all! r805189
        Hide
        Mark Miller added a comment -

        patch with change + Changes entries

        not sure if we want to add an expert warning as well or not ...

        Show
        Mark Miller added a comment - patch with change + Changes entries not sure if we want to add an expert warning as well or not ...
        Hide
        Mark Miller added a comment -

        If no one objects, I will make this change in a day or two.

        Show
        Mark Miller added a comment - If no one objects, I will make this change in a day or two.
        Hide
        Mark Miller added a comment -

        Okay, thats a good argument - no objection from me. I'll open it unless anyone objects.

        Show
        Mark Miller added a comment - Okay, thats a good argument - no objection from me. I'll open it unless anyone objects.
        Hide
        Shai Erera added a comment -

        Nice catch. I forgot Weight was changed to abstract class. +1 from me.

        Show
        Shai Erera added a comment - Nice catch. I forgot Weight was changed to abstract class. +1 from me.
        Hide
        Tim Smith added a comment -

        Here's an option

        just make createWeight() public

        this will require anyone who used it to have their subclass of query open it up public and recompile

        however, if someone implemented createWeight() in <=2.4 and are going to 2.9, they already have to do this (because Weight was changed from an interface to an abstract class, so they have to do a similar change already:

        private class *Weight implements Weight {
        

        already has to be changed to

        private class *Weight extends Weight {
        

        Anyone (almost anyone) who implements createWeight() will already be hit by this back compat break, so why not just make createWeight() public at the same time Weight was changed from an interface to an abstract class

        Show
        Tim Smith added a comment - Here's an option just make createWeight() public this will require anyone who used it to have their subclass of query open it up public and recompile however, if someone implemented createWeight() in <=2.4 and are going to 2.9, they already have to do this (because Weight was changed from an interface to an abstract class, so they have to do a similar change already: private class *Weight implements Weight { already has to be changed to private class *Weight extends Weight { Anyone (almost anyone) who implements createWeight() will already be hit by this back compat break, so why not just make createWeight() public at the same time Weight was changed from an interface to an abstract class
        Hide
        Shai Erera added a comment -

        I thought that's partly we took care of here: https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12723996&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12723996

        True, if someone overrides createWeight (he ought to) and call it specifically, createQueryWeight won't be called. But then, all of our code will call createQueryWeight. And if we deprecate createWeight, those who call it directly will need to move to createQueryWeight, so I think we should be fine?

        Anyway, I may not think too clear at this hour (1 AM), so if I misunderstood something, I'll read it again in the morning.

        Show
        Shai Erera added a comment - I thought that's partly we took care of here: https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12723996&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12723996 True, if someone overrides createWeight (he ought to) and call it specifically, createQueryWeight won't be called. But then, all of our code will call createQueryWeight. And if we deprecate createWeight, those who call it directly will need to move to createQueryWeight, so I think we should be fine? Anyway, I may not think too clear at this hour (1 AM), so if I misunderstood something, I'll read it again in the morning.
        Hide
        Mark Miller added a comment - - edited

        Done you have the above problem though:

        If you add createQueryWeight as a public method, then all of the Lucene classes have to be changed to call it - otherwise, if you override it in a user Query, it won't be called on that Query.

        But anyone with an external Query class that calls [<-FIXED] createWeight will not call createQueryWeight, and won't work correctly with classes that override it. I guess if we make it final it would close that loop hole, but then thats a loss from createWeight where you could override, and is still a back compat break?

        Show
        Mark Miller added a comment - - edited Done you have the above problem though: If you add createQueryWeight as a public method, then all of the Lucene classes have to be changed to call it - otherwise, if you override it in a user Query, it won't be called on that Query. But anyone with an external Query class that calls [<-FIXED] createWeight will not call createQueryWeight, and won't work correctly with classes that override it. I guess if we make it final it would close that loop hole, but then thats a loss from createWeight where you could override, and is still a back compat break?
        Hide
        Shai Erera added a comment -

        When I changed createQueryWeight from protected to public, it was because we introduced it in 2.9 only, so it was possible. Perhaps we should deprecate createWeight, and add back createQueryWeight as public?

        Show
        Shai Erera added a comment - When I changed createQueryWeight from protected to public, it was because we introduced it in 2.9 only, so it was possible. Perhaps we should deprecate createWeight, and add back createQueryWeight as public?
        Hide
        Mark Miller added a comment -

        Ahh - nice catch. I'm not sure what to do here then...

        The previous possible break (I didn't actually look into it so I dunno) was referenced here:

        http://search.lucidimagination.com/search/document/41004a9436799675/spanquery_and_boostingtermquery_oddities

        Show
        Mark Miller added a comment - Ahh - nice catch. I'm not sure what to do here then... The previous possible break (I didn't actually look into it so I dunno) was referenced here: http://search.lucidimagination.com/search/document/41004a9436799675/spanquery_and_boostingtermquery_oddities
        Hide
        Shai Erera added a comment -

        I can always extend a class and make a package-private or protected method public. I cannot reduce visibility, but can always increase it.

        Ohh ... after hitting Submit I understood why it would break back-compat - if I extend Query and override createWeight, and leave it 'protected' I won't compile if we make it public, since I'll be reducing visibility.

        Show
        Shai Erera added a comment - I can always extend a class and make a package-private or protected method public. I cannot reduce visibility, but can always increase it. Ohh ... after hitting Submit I understood why it would break back-compat - if I extend Query and override createWeight, and leave it 'protected' I won't compile if we make it public, since I'll be reducing visibility.
        Hide
        Shai Erera added a comment -

        Can't you open up visibility without breaking back compat?

        I don't see why this would break back-compat. I can always extend a class and make a package-private or protected method public. I cannot reduce visibility, but can always increase it.

        About the issues w/ createQueryWeight, I think you're referring to the chain of comments that started here: https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12723976&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12723976. Is that what you were talking about?

        Show
        Shai Erera added a comment - Can't you open up visibility without breaking back compat? I don't see why this would break back-compat. I can always extend a class and make a package-private or protected method public. I cannot reduce visibility, but can always increase it. About the issues w/ createQueryWeight, I think you're referring to the chain of comments that started here: https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12723976&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12723976 . Is that what you were talking about?
        Hide
        Mark Miller added a comment -

        1. make createWeight() public on Query (breaks back compat)

        hmmm - I took that as fact, but is that true? Can't you open up visibility without breaking back compat? Time to look this stuff up again ...

        Show
        Mark Miller added a comment - 1. make createWeight() public on Query (breaks back compat) hmmm - I took that as fact, but is that true? Can't you open up visibility without breaking back compat? Time to look this stuff up again ...
        Hide
        Mark Miller added a comment -

        I havn't yet figured out how to do this without breaking back compat - I think this was an issue before as well. I'd have to dig it up, but some user complained about a similar issue when QueryWeight was put in.

        If you add createQueryWeight as a public method, then all of the Lucene classes have to be changed to call it - otherwise, if you override it in a user Query, it won't be called on that Query.

        But anyone with an external Query class that overrode createWeight will not call createQueryWeight, and won't work correctly with classes that override it. I guess if we make it final it would close that loop hole, but then thats a loss from createWeight where you could override, and is still a back compat break?

        Show
        Mark Miller added a comment - I havn't yet figured out how to do this without breaking back compat - I think this was an issue before as well. I'd have to dig it up, but some user complained about a similar issue when QueryWeight was put in. If you add createQueryWeight as a public method, then all of the Lucene classes have to be changed to call it - otherwise, if you override it in a user Query, it won't be called on that Query. But anyone with an external Query class that overrode createWeight will not call createQueryWeight, and won't work correctly with classes that override it. I guess if we make it final it would close that loop hole, but then thats a loss from createWeight where you could override, and is still a back compat break?
        Hide
        Mark Miller added a comment -

        Of course, that was just a comment.

        I never leave a comment alone Gotto inflate my JIRA stats somehow.

        Show
        Mark Miller added a comment - Of course, that was just a comment. I never leave a comment alone Gotto inflate my JIRA stats somehow.
        Hide
        Shai Erera added a comment -

        when you made createQueryWeight, you made it public

        Found it - https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12719992&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12719992 - I originally created it protected, only later made it public, following Yonik's proposal from here - http://osdir.com/ml/java-dev.lucene.apache.org/2009-06/msg00803.html.

        things are always changing, and new info / tradeoffs always come up.

        Of course, that was just a comment.

        Show
        Shai Erera added a comment - when you made createQueryWeight, you made it public Found it - https://issues.apache.org/jira/browse/LUCENE-1630?focusedCommentId=12719992&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12719992 - I originally created it protected, only later made it public, following Yonik's proposal from here - http://osdir.com/ml/java-dev.lucene.apache.org/2009-06/msg00803.html . things are always changing, and new info / tradeoffs always come up. Of course, that was just a comment.
        Hide
        Mark Miller added a comment -

        it could have saved me a lot of time creating this QueryWeight replacement

        And it would have saved me time reverting and making it an abstract class - what can you do though? If we could have kept back compat here, I think we would have - but we didn't find a good way that avoided the issues we faced.

        I think there was a user that picked up a back compat break with QueryWeight anyway - a method he could override wasn't being called anymore? So its prob for the best - things are always changing, and new info / tradeoffs always come up.

        Show
        Mark Miller added a comment - it could have saved me a lot of time creating this QueryWeight replacement And it would have saved me time reverting and making it an abstract class - what can you do though? If we could have kept back compat here, I think we would have - but we didn't find a good way that avoided the issues we faced. I think there was a user that picked up a back compat break with QueryWeight anyway - a method he could override wasn't being called anymore? So its prob for the best - things are always changing, and new info / tradeoffs always come up.
        Hide
        Mark Miller added a comment -

        I created createQueryWeight as a replacement to createWeight

        createWeight was protected - when you made createQueryWeight, you made it public - so that was the change I think.

        Show
        Mark Miller added a comment - I created createQueryWeight as a replacement to createWeight createWeight was protected - when you made createQueryWeight, you made it public - so that was the change I think.
        Hide
        Shai Erera added a comment -

        Query always had a public weight() method, and protected createWeight method. I don't remember that I touched it in LUCENE-1630. I created createQueryWeight as a replacement to createWeight.

        BTW, I completely missed LUCENE-1771. I wish we had decided that when I was working on 1630 - it could have saved me a lot of time creating this QueryWeight replacement .

        Show
        Shai Erera added a comment - Query always had a public weight() method, and protected createWeight method. I don't remember that I touched it in LUCENE-1630 . I created createQueryWeight as a replacement to createWeight. BTW, I completely missed LUCENE-1771 . I wish we had decided that when I was working on 1630 - it could have saved me a lot of time creating this QueryWeight replacement .
        Hide
        Tim Smith added a comment -

        Right now, i have to use the following class in org.apache.lucene.search package in order to work around this:

        /**
         * Static utility class for exposing some otherwise protected stuff.
         */
        public final class SearchUtils {
          /**
           * Expose protected Query.createWeight(Searcher) method.
           */
          public static Weight createWeight(Searcher searcher, Query query) throws IOException {
            return query.createWeight(searcher);
          }
        }
        

        ideally, i wouldn't need this class

        Show
        Tim Smith added a comment - Right now, i have to use the following class in org.apache.lucene.search package in order to work around this: /** * Static utility class for exposing some otherwise protected stuff. */ public final class SearchUtils { /** * Expose protected Query.createWeight(Searcher) method. */ public static Weight createWeight(Searcher searcher, Query query) throws IOException { return query.createWeight(searcher); } } ideally, i wouldn't need this class
        Hide
        Michael McCandless added a comment -

        I don't remember why we had decided (when adding QueryWeight) to open it up, but, I think opening it up makes sense for advanced use cases?

        Show
        Michael McCandless added a comment - I don't remember why we had decided (when adding QueryWeight) to open it up, but, I think opening it up makes sense for advanced use cases?
        Hide
        Mark Miller added a comment -

        Ah, I didn't catch that one - we did reduce the previously opened visibility here.

        Shai, Mike? We should open the visibility on this back up? I'm not familiar with the issue that made it public.

        Show
        Mark Miller added a comment - Ah, I didn't catch that one - we did reduce the previously opened visibility here. Shai, Mike? We should open the visibility on this back up? I'm not familiar with the issue that made it public.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Tim Smith
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development