Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 4.9, 5.0
    • Component/s: general/javadocs
    • Labels:
    • Lucene Fields:
      New, Patch Available

      Description

      I have fixed the javadoc for "Modified Score" formular in CustomScoreQuery. - Patch attached: customScoreQuery_JavaDoc.patch

      I'm quite curious why the method:
      public float customScore(int doc, float subQueryScore, float valSrcScores[])

      calls public float customScore(int doc, float subQueryScore, float valSrcScore]) only in 2 of the 3 cases which makes the choice to override either one of the customScore methods dependent on the number of ValueSourceQuery passed to the constructor. I figure it would be more consistent if it would call the latter in all 3 cases.

      I also attached a patch which proposes a fix for that issue. The patch does also include the JavaDoc issue mentioned above.

      • customScoreQuery_CodeChange+JavaDoc.patch
      1. LUCENE-1650.patch
        4 kB
        Mark Miller
      2. LUCENE-1650.patch
        4 kB
        Mark Miller
      3. customScoreQuery_JavaDoc.patch
        0.6 kB
        Simon Willnauer
      4. customScoreQuery_CodeChange+JavaDoc.patch
        2 kB
        Simon Willnauer

        Activity

        Hide
        Mark Miller added a comment -

        This looks like it makes sense to me. Unless anyone objects, I'd like to commit it for 2.9.

        Show
        Mark Miller added a comment - This looks like it makes sense to me. Unless anyone objects, I'd like to commit it for 2.9.
        Hide
        Mark Miller added a comment -

        Im going to commit this soon, both changes.

        Show
        Mark Miller added a comment - Im going to commit this soon, both changes.
        Hide
        Mark Miller added a comment -

        No I'm not Yonik, could you take a peak at this?

        Why the javadoc change? It used to refer to the method parameter - that makes sense to me ? Where is valSrcScore bound?

        As far as the second change, its a logical change, but it is a back compat change. I'm thinking it could be considered a bug fix, but the old weirdness worked if
        you followed the javadoc no?

        I'm pro the change, and it prob doesnt make sense to add a static or property to allow the old behaviour, but another opinion...

        Show
        Mark Miller added a comment - No I'm not Yonik, could you take a peak at this? Why the javadoc change? It used to refer to the method parameter - that makes sense to me ? Where is valSrcScore bound? As far as the second change, its a logical change, but it is a back compat change. I'm thinking it could be considered a bug fix, but the old weirdness worked if you followed the javadoc no? I'm pro the change, and it prob doesnt make sense to add a static or property to allow the old behaviour, but another opinion...
        Hide
        Mark Miller added a comment -

        updated to trunk in any case.

        Show
        Mark Miller added a comment - updated to trunk in any case.
        Hide
        Yonik Seeley added a comment -

        Not sure why you wanted me to take a peek - this isn't part of the original Solr function query stuff, so I won't know any more than anyone else.
        Anyway the current code looks like it's working as designed? Perhaps it wasn't the best interface, but not worth breaking compatibility over, and not necessary to improve for 2.9 IMO.

        Show
        Yonik Seeley added a comment - Not sure why you wanted me to take a peek - this isn't part of the original Solr function query stuff, so I won't know any more than anyone else. Anyway the current code looks like it's working as designed? Perhaps it wasn't the best interface, but not worth breaking compatibility over, and not necessary to improve for 2.9 IMO.
        Hide
        Mark Miller added a comment -

        Not sure why you wanted me to take a peek - this isn't part of the original Solr function query stuff, so I won't know any more than anyone else.

        Because you knew that, and I didnt And because your pretty smart about these things. And your usually happy to ignore when you are not interested, so I figured you always had that out here I think there are usually okay odds that you might know more than someone else.

        Anyway the current code looks like it's working as designed? Perhaps it wasn't the best interface, but not worth breaking compatibility over, and not necessary to improve for 2.9 IMO.

        That's kind of what I started thinking, and I just wanted someone smarter than me to confirm or deny. It also just seemed like it was very unlikely that it would affect anyone if the change was made, so why not just make it consistent. I could go either way, but technically it is a backcompat break, and it would be easier to just leave it.

        I'll just push it off 2.9 for now.

        I'm just trying to resolve/push 2.9 issues - didnt mean to single you out for any specific work

        Show
        Mark Miller added a comment - Not sure why you wanted me to take a peek - this isn't part of the original Solr function query stuff, so I won't know any more than anyone else. Because you knew that, and I didnt And because your pretty smart about these things. And your usually happy to ignore when you are not interested, so I figured you always had that out here I think there are usually okay odds that you might know more than someone else. Anyway the current code looks like it's working as designed? Perhaps it wasn't the best interface, but not worth breaking compatibility over, and not necessary to improve for 2.9 IMO. That's kind of what I started thinking, and I just wanted someone smarter than me to confirm or deny. It also just seemed like it was very unlikely that it would affect anyone if the change was made, so why not just make it consistent. I could go either way, but technically it is a backcompat break, and it would be easier to just leave it. I'll just push it off 2.9 for now. I'm just trying to resolve/push 2.9 issues - didnt mean to single you out for any specific work
        Hide
        Steve Rowe added a comment -

        Bulk move 4.4 issues to 4.5 and 5.0

        Show
        Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
        Hide
        Uwe Schindler added a comment -

        Move issue to Lucene 4.9.

        Show
        Uwe Schindler added a comment - Move issue to Lucene 4.9.

          People

          • Assignee:
            Unassigned
            Reporter:
            Simon Willnauer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development