SOLR-15449: edismax sow and mm #158

alessandrobenedetti · 2021-06-02T10:45:55Z

https://issues.apache.org/jira/browse/SOLR-15449

Description

When a field text analysis is incompatible with the query text, mm is not fully respected:

sow = false
mm=100%
qf = text numeric_i
q = terminator 100
defType = edismax
"parsedquery_toString":
"+(((text:terminator text:100)~2) | 
(numeric_i:100)~1))"
A document just containing '100' in the field numeric_i is returned as a good search result but it actually doesn't respect the mm=100%

Solution

Instead of just ignoring un-parsable clauses, a noMatch query is added

Tests

tests have been added to: org.apache.solr.search.TestExtendedDismaxParser

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

… fix for not compatible query term-field + tests

alessandrobenedetti · 2021-06-02T10:49:32Z

@dsmiley @janhoy @madrob @munendrasn @romseygeek @sarowe , the issue with sow and mm found in: #129 has been isolated here and it's ready for an initial review.
Once approved I'll move to add CHANGES.txt and other burocracy stuff

dsmiley

Other than formatting matters, I think this is good!

solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java

dsmiley · 2021-06-02T13:24:57Z

solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java

@@ -1776,6 +1764,36 @@ private static String getParsedQuery(SolrQueryRequest request) throws Exception
    return (String) BaseTestHarness.evaluateXPath(resp, "//str[@name='parsedquery']/text()", XPathConstants.STRING);
  }

+  public void testSplitOnWhitespace_shouldRespectMinimumShouldMatch(){


The indentation here is inconsistent. Please configure your IDE so that this simply doesn't happen.

I think I've done it, formatting is one of the things I hate the most, my opinion is that it shouldn't be the responsibility of single peers but it should be server side to avoid inconsistencies(which I often find also in Lucene/Solr and other Open Source code).
Never really explored a solution for that either, so I hope, my changes are ok now :)

https://issues.apache.org/jira/browse/SOLR-14920 -- I anticipate this will happen for 9.0 soon.

solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java

solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java

magibney · 2021-06-03T02:14:58Z

solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java

-    // When sow=false, the per-field query structures differ (no "Terminator" query on integer field foo_i),
-    // so a dismax-per-field is constructed.  As a result, mm=100% is applied per-field instead of per-term;
-    // since there is only one term (100) required in the foo_i field's dismax, the query can match docs that
-    // only have the 100 term in the foo_i field, and don't necessarily have "Terminator" in any field.


@sarowe, the above comment (from dd171ff) describes behavior that seems counterintuitive, but it doesn't give any indication of whether the behavior might be considered desirable for some reason? The behavior described would be changed by this PR; this seems like it would be an improvement, but I wanted to call your attention to this comment specifically, just to make sure you're aware/supportive of the change.

alessandrobenedetti · 2021-06-03T10:18:21Z

As soon as I see an approval and we get potentially a final feedback by @sarowe , I am happy to merge.

For cherry picking, I was just taking a look to the minor version branches but I couldn't find them, has anything changed after the split?
i.e. I was looking for example for 8.9.X branch or 8.10.x (will this exist?) but I couldn't find any

janhoy · 2021-06-03T12:33:42Z

The new apache/solr repo only contains main branch, and will eventually contain branch_9x etc.
To backport to 8.x you will need to port the change over to the https://github.com/apache/lucene-solr repo (see README there), that's where the branch_8x lives.
It's not that hard. What I have done is to add apache/solr as a remote to my apache/lucene-solr checkout, so I can do a git fetch and then cherry-pick the main-branch commit into my old repo.

alessandrobenedetti · 2021-06-07T15:39:19Z

@dsmiley I see that changes are still requested, can you check that?
I would like to progress with the merge, I doubt we get any other feedback here.

dsmiley

Thanks Alessandro!

# Conflicts: # solr/CHANGES.txt

If we fail to delete files that belong to a commit point, then we will expose that deleted commit in the next calls of IndexDeletionPolicy#onCommit. I think we should never expose those deleted commit points as some of their files might have been deleted already.

alessandrobenedetti added 10 commits May 14, 2021 17:03

[SEARCH-15407] untokenized field type with sow=false fix + tests

3f2b5a8

[SEARCH-15407] check on StrField to avoid unwanted tokenization + bug…

4ef9afe

… fix for not compatible query term-field + tests

[SEARCH-15407] check on StrField to avoid unwanted tokenization + bug…

f882e45

… fix for not compatible query term-field + tests

[SEARCH-15407] minor

e6c079a

[SEARCH-15407] added a test for numerical

660d28a

[SEARCH-15407] added a comment for numerical parsing exceptions

32b9301

[SEARCH-15407] added a comment for numerical parsing exceptions

b52e5aa

[SEARCH-15407] tests rename

8679ce2

Merge remote-tracking branch 'origin/main' into jira/solr-15449

2c0d864

[SEARCH-15449] sow and mm fix

24c0a57

alessandrobenedetti requested review from dsmiley, janhoy, madrob, munendrasn, romseygeek and sarowe June 2, 2021 10:47

alessandrobenedetti mentioned this pull request Jun 2, 2021

SOLR-15407 untokenized field type with sow=false fix + tests #129

Closed

dsmiley requested changes Jun 2, 2021

View reviewed changes

[SEARCH-15449] formatting

3f140bf

magibney reviewed Jun 2, 2021

View reviewed changes

solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java Outdated Show resolved Hide resolved

magibney reviewed Jun 2, 2021

View reviewed changes

solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java Outdated Show resolved Hide resolved

magibney reviewed Jun 3, 2021

View reviewed changes

alessandrobenedetti added 2 commits June 3, 2021 11:07

Merge remote-tracking branch 'origin/main' into jira/solr-15449

dcd451e

[SEARCH-15449] changes.txt + minor empty constructor init

3349378

alessandrobenedetti added 2 commits June 3, 2021 15:01

[SEARCH-15449] minor comment

d92927e

[SEARCH-15449] minor comment

ca6c9d7

dsmiley approved these changes Jun 7, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into jira/solr-15449

7de4cd6

# Conflicts: # solr/CHANGES.txt

alessandrobenedetti merged commit 9791057 into main Jun 9, 2021

alessandrobenedetti deleted the jira/solr-15449 branch June 9, 2021 11:02

epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021

SOLR-15449: edismax sow and mm (apache#158)

103c884

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SOLR-15449: edismax sow and mm #158

SOLR-15449: edismax sow and mm #158

Uh oh!

alessandrobenedetti commented Jun 2, 2021

Uh oh!

alessandrobenedetti commented Jun 2, 2021

Uh oh!

dsmiley left a comment

Uh oh!

Uh oh!

dsmiley Jun 2, 2021

Uh oh!

alessandrobenedetti Jun 2, 2021

Uh oh!

dsmiley Jun 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

magibney Jun 3, 2021

Uh oh!

alessandrobenedetti commented Jun 3, 2021

Uh oh!

janhoy commented Jun 3, 2021

Uh oh!

alessandrobenedetti commented Jun 7, 2021

Uh oh!

dsmiley left a comment

Uh oh!

Uh oh!

SOLR-15449: edismax sow and mm #158

SOLR-15449: edismax sow and mm #158

Uh oh!

Conversation

alessandrobenedetti commented Jun 2, 2021

Description

Solution

Tests

Checklist

Uh oh!

alessandrobenedetti commented Jun 2, 2021

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsmiley Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

alessandrobenedetti Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

dsmiley Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

magibney Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

alessandrobenedetti commented Jun 3, 2021

Uh oh!

janhoy commented Jun 3, 2021

Uh oh!

alessandrobenedetti commented Jun 7, 2021

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!