Solr
  1. Solr
  2. SOLR-5882

Introduce score local parameter for {!parent} query parser

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.8
    • Fix Version/s: 5.3
    • Component/s: None
    • Labels:
      None

      Description

      I propose to have ability to configure scoring mode by optional local parameter

      Syntax for parent queries is:

      {!parent which=type:parent score=none|avg|max|total|min}...
      Capital case for score values is also accepted
      

      Child query enables scoring by default.

      1. SOLR-5882.patch
        14 kB
        Mikhail Khludnev
      2. SOLR-5882.patch
        5 kB
        Andrey Kudryavtsev

        Issue Links

          Activity

          Hide
          Andrey Kudryavtsev added a comment -

          Initial patch

          Show
          Andrey Kudryavtsev added a comment - Initial patch
          Hide
          ash fo added a comment -

          How do I apply this patch on windows? I am using Bitnami Solr 4.10.2 on windows. Thanks.

          Show
          ash fo added a comment - How do I apply this patch on windows? I am using Bitnami Solr 4.10.2 on windows. Thanks.
          Hide
          Andrey Kudryavtsev added a comment -

          Use one of Windows GUI utilities for example. More details - http://stackoverflow.com/questions/517257/how-do-i-apply-a-diff-patch-on-windows

          Show
          Andrey Kudryavtsev added a comment - Use one of Windows GUI utilities for example. More details - http://stackoverflow.com/questions/517257/how-do-i-apply-a-diff-patch-on-windows
          Hide
          Alessandro Benedetti added a comment -

          Hi,
          Is there any idea when this patch will be included in Solr Code Base ?
          I find it to be a very interesting aspect to complete the Join feature!

          Cheers

          Show
          Alessandro Benedetti added a comment - Hi, Is there any idea when this patch will be included in Solr Code Base ? I find it to be a very interesting aspect to complete the Join feature! Cheers
          Hide
          Mikhail Khludnev added a comment -

          Andrey Kudryavtsev would you mind to make sure that patch suites for trunk?

          Show
          Mikhail Khludnev added a comment - Andrey Kudryavtsev would you mind to make sure that patch suites for trunk?
          Hide
          Mikhail Khludnev added a comment -

          QParser test found a bug in ToParentBlockJoinQuery.BlockJoinScorer.nextDoc() it occurs on ScoreMode.Min

          minScore = Math.min(childFreq, minScore); // shouldn't it be childScore, not Freq?
          

          I attach a patch includes this fix and test coverage.

          Show
          Mikhail Khludnev added a comment - QParser test found a bug in ToParentBlockJoinQuery.BlockJoinScorer.nextDoc() it occurs on ScoreMode.Min minScore = Math .min(childFreq, minScore); // shouldn't it be childScore, not Freq? I attach a patch includes this fix and test coverage.
          Hide
          Mikhail Khludnev added a comment -

          attaching patch.

          • introduce {!parent score=..}
          • it turns out, {!child} already returns scores always
          • introduced util for parsing score enum, it also made query-time join parser more correct, it throws SolrException on wrong enum name.
          • fix ScoreMode.Min at ToParentBlockJoin and test it

          I'm kindly ask to review. Due to the last item, I want to commit it before 5.3 cut.

          Show
          Mikhail Khludnev added a comment - attaching patch. introduce {!parent score=..} it turns out, {!child} already returns scores always introduced util for parsing score enum, it also made query-time join parser more correct, it throws SolrException on wrong enum name. fix ScoreMode.Min at ToParentBlockJoin and test it I'm kindly ask to review. Due to the last item, I want to commit it before 5.3 cut.
          Hide
          Shalin Shekhar Mangar added a comment -

          Mikhail Khludnev - does a score mode specified in uppercase throw an exception? Shouldn't that be case-insensitive?

          Show
          Shalin Shekhar Mangar added a comment - Mikhail Khludnev - does a score mode specified in uppercase throw an exception? Shouldn't that be case-insensitive?
          Hide
          Mikhail Khludnev added a comment -

          Shalin Shekhar Mangar it fails on upper case, it accepts only Capital and lowercase. Here it mimics SOLR-6234 behavior. I just didn't find a guide how to do that. Is there any examples of case insensitive behavior of qparsers?
          If you prefer I can rework ScoreModeParser which can make both qparsers case insensitive for scoremode, it's not a big deal.

          Show
          Mikhail Khludnev added a comment - Shalin Shekhar Mangar it fails on upper case, it accepts only Capital and lowercase. Here it mimics SOLR-6234 behavior. I just didn't find a guide how to do that. Is there any examples of case insensitive behavior of qparsers? If you prefer I can rework ScoreModeParser which can make both qparsers case insensitive for scoremode, it's not a big deal.
          Hide
          Shalin Shekhar Mangar added a comment -

          Okay, I don't have a serious objection to this but I just find it weird to require case-sensitivity for such parameter names. Perhaps we can use lowercase only?

          Show
          Shalin Shekhar Mangar added a comment - Okay, I don't have a serious objection to this but I just find it weird to require case-sensitivity for such parameter names. Perhaps we can use lowercase only?
          Hide
          Shalin Shekhar Mangar added a comment -

          Oh and rest of the patch looks good to me, thanks!

          Show
          Shalin Shekhar Mangar added a comment - Oh and rest of the patch looks good to me, thanks!
          Hide
          Mikhail Khludnev added a comment -

          lowercase works. and it's up to user to leave the first letter Capital like in enum's javadoc

          Show
          Mikhail Khludnev added a comment - lowercase works. and it's up to user to leave the first letter Capital like in enum's javadoc
          Hide
          Shalin Shekhar Mangar added a comment -

          Okay, cool. +1 to commit!

          Show
          Shalin Shekhar Mangar added a comment - Okay, cool. +1 to commit!
          Hide
          ASF subversion and git services added a comment -

          Commit 1693926 from mkhl@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1693926 ]

          SOLR-5882: introducing local param

          {!parent score=..}

          ..
          fixing ScoreMode.Min for ToParentBlockJoinQuery
          fixing ScoreMode parsing exception in ScoreJoinQParserPlugin

          Show
          ASF subversion and git services added a comment - Commit 1693926 from mkhl@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1693926 ] SOLR-5882 : introducing local param {!parent score=..} .. fixing ScoreMode.Min for ToParentBlockJoinQuery fixing ScoreMode parsing exception in ScoreJoinQParserPlugin
          Hide
          Yonik Seeley added a comment -

          > Okay, I don't have a serious objection to this but I just find it weird to require case-sensitivity for such parameter names. Perhaps we can use lowercase only?

          lowercase works. and it's up to user to leave the first letter Capital like in enum's javadoc

          I agree with Shalin, and I don't think the enum value is really relevant (again, interface vs implementation... plus someone could always change the enum).
          So when this is documented in the ref guide, let's use lowecase.

          Show
          Yonik Seeley added a comment - > Okay, I don't have a serious objection to this but I just find it weird to require case-sensitivity for such parameter names. Perhaps we can use lowercase only? lowercase works. and it's up to user to leave the first letter Capital like in enum's javadoc I agree with Shalin, and I don't think the enum value is really relevant (again, interface vs implementation... plus someone could always change the enum). So when this is documented in the ref guide, let's use lowecase.
          Hide
          ASF subversion and git services added a comment -

          Commit 1693941 from mkhl@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1693941 ]

          SOLR-5882: introducing local param

          {!parent score=..}

          ..
          fixing ScoreMode.Min for ToParentBlockJoinQuery
          fixing ScoreMode parsing exception in ScoreJoinQParserPlugin

          Show
          ASF subversion and git services added a comment - Commit 1693941 from mkhl@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1693941 ] SOLR-5882 : introducing local param {!parent score=..} .. fixing ScoreMode.Min for ToParentBlockJoinQuery fixing ScoreMode parsing exception in ScoreJoinQParserPlugin
          Hide
          Mikhail Khludnev added a comment -

          Cassandra Targett note, I mentioned it in the ref guide. It should be fine, because I copypasted your paragraph.

          Show
          Mikhail Khludnev added a comment - Cassandra Targett note, I mentioned it in the ref guide. It should be fine, because I copypasted your paragraph.
          Hide
          Mikhail Khludnev added a comment -
          Show
          Mikhail Khludnev added a comment - Thanks, Andrey Kudryavtsev !
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

            People

            • Assignee:
              Mikhail Khludnev
              Reporter:
              Andrey Kudryavtsev
            • Votes:
              11 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development