You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/05/14 16:08:38 UTC
[GitHub] [solr] alessandrobenedetti opened a new pull request #129: [SEARCH-15407] untokenized field type with sow=false fix + tests
alessandrobenedetti opened a new pull request #129:
URL: https://github.com/apache/solr/pull/129
# Description
sow=false should not split on whitespace and build field queries with the entire query text.
It's the responsibility of each field query text analysis to manage tokenization if any.
Currently this doesn't happen for not textual fields such as string, numerical.
It's up to discussion if we want to apply tokenization to numerical, even with sow=false
# Solution
no tokenization happens if the field is not textual
# Tests
There are old tests that have been corrected as their behaviour was incorrect, with sow=false in my opinion.
There is a org.apache.solr.search.TestExtendedDismaxParser#testFocusQueryParser that is up for discussion in relation to numerical fields.
# Checklist
Please review the following and check all that apply:
- [ X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ X] I have created a Jira issue and added the issue ID to my pull request title.
- [ X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ X] I have developed this patch against the `main` branch.
- [X ] I have run `./gradlew check`.
- [X ] I have added tests for my changes.
- [X ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-852925576
Sister issue and PR has been added:
https://issues.apache.org/jira/browse/SOLR-15449
https://github.com/apache/solr/pull/158
I am going now to clean this PR just leaving the string field part
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853380214
> ** Current Incorrect Behaviour **
> If I query a "StrField" type or a text field type Keyword Tokenized, I get two different behaviours :
> sow=false -> it splits on whitespace anyway for StrField, causing incorrect results, for Keyword Tokenized texts, it works correctly
> sow=true -> to split on whitespace independently of the field type, it works correctly
This summary is helpful. If starting with a clean slate, perhaps the "correct" behavior should be as you've outlined -- but then I would think you'd want the same strict "non-splitting" behavior for integer fields. My gut reaction is that this is a very unusual use case (configuring a StrField or integer field as `qf`). In fact, a strong argument in principal for "strict" non-splitting behavior would be to effectively discourage this kind of configuration in the first place!
Ultimately though I think I'm converging on agreement with https://github.com/apache/solr/pull/129/files#r638071458 ... tbh I'm having a hard time imagining a use case where the configuration in question (or its current behavior) would be desirable; but assuming there may be such use cases already in place, this change could surely disrupt them.
Perhaps the fact that keyword-tokenized text fields currently work "correctly" (as defined above) could be employed to address this issue going forward for certain use-cases, rather than changing existing behavior? Keyword-tokenized text would also seem more appropriate to the `edismax` use-case, by providing analysis for text _normalization_ (the lack of which strikes me as the most unusual aspect of configuring StrField `qf`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853836054
The core of my point is that that Solr *already* has an API to interrogate the field type to see if it produces one token or more -- {{FieldType.isTokenized}}. SolrQueryParserBase uses this in a number of places already. It appears you want to make StrField a further specialized case when isTokenized==false is not distinguishing enough and that appears to me going too far / hacky. It might be justifiable if we felt the result was clearly the correct behavior but we don't even think that (I don't, nor does Michael).
Nonetheless some user control could be added so that someone with a normalizing text field that uses TextField (likely with KeywordTokenizer but not necessarily) could get query parser behavior that is the same as how StrField behaves. It'd be nice if a user could set `tokenized="false"` on the fieldType definition that uses TextField if they can attest the processing doesn't tokenize. I experimented this morning with that and was able to do it by modifying the first line of TextField.init to be this snippet:
```
if ((falseProperties & TOKENIZED) == 0) {
properties |= TOKENIZED;
}
```
(instead of insisting TOKENIZED as it did before).
And maybe the existing logic in SolrQueryParserBase could be improved somehow; I'm definitely not claiming it's perfection. It just strikes me as a cast on StrField is a strong code smell to me that we're doing something wrong when there is already isTokenized which seems suitable for your purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti edited a comment on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti edited a comment on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857805113
Making an update here to understand the potential next steps.
So I think we agree that:
1) if you configure a StrField in the QF and you configure sow=false, currently the parameter is ignored, you split by whitespace and you don't match your multi terms (it is like we use a query time tokenization, not in line with the expected invisible query time keyword tokenization of the StrField.) This is incorrect and gets you zero results when you shouldn't
2) the StrField condition is not a solution you like, so we won't approve the pull request in the current state
So next steps:
3) it seems we want to "discourage" the usage of StrField in QF with an inconsistent behaviour (in comparison with a text field with the same keyword tokenization analysis). Should we then explicitly raise an error if the user passes a StrField (maybe all non-textual fields) in QF?
OR
4) should we remove the StrField type completely? we can leave the schema.xml back-compatible, but always use text fields for textual information server-side (I think it's the same approach that Elasticsearch uses and it makes sense to me)
OR
5) just ignore this issue? I am not a fan of this
OR
6) any other idea? super happy to explore other options
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-847276421
In the meantime I was still thinking about this and I still think it is a
bug:
If we set a field type to be keyword analysed (so producing the same token
as a String field), the sow works correctly and we have the same behaviour
I am introducing with the fix.
On Mon, 24 May 2021, 16:59 David Smiley, ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> It seems we don't agree on this yet.
> ------------------------------
>
> In solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
> <https://github.com/apache/solr/pull/129#discussion_r637963606>:
>
> > @@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
> assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
> }
>
> + @Test
> + public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
>
> Based on the test name, I'd expect sow=true each time. Maybe just drop
> this part of the method name.
> ------------------------------
>
> In solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
> <https://github.com/apache/solr/pull/129#discussion_r637963928>:
>
> > @@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
> assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
> }
>
> + @Test
> + public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
> + {
> + assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
> + "/response/numFound==1", "/response/docs/[0]/id=='75'");
> +
> + String parsedquery = getParsedQuery(
> + req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
> + assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
> + }
> +
> + @Test
> + public void testSplitOnWhitespace_numericField_shouldBuildAlwaysMultiClause() throws Exception
>
> Again, just drop "testSplitOnWhitespace_" from the method name, I think.
> ------------------------------
>
> In solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
> <https://github.com/apache/solr/pull/129#discussion_r638071458>:
>
> > @@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
> assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
> }
>
> + @Test
> + public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
> + {
> + assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
>
> This is a change in behavior, and I think it's not a good change. For a
> non-tokenized field (StrField in this case), I think we should ignore
> whatever "sow" is and split on whitespace any way, thus here have two terms
> to match. It would be straight-forward to document this (no differences
> between numbers and StrField).
>
> I think it could be reasonable to try both ways (both split and don't
> split) and then put a DisjunctionMaxQuery over the two, though I'd prefer
> not.
> ------------------------------
>
> In solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
> <https://github.com/apache/solr/pull/129#discussion_r638074368>:
>
> > } else {
> List<Query> subqs = new ArrayList<>();
> for (String queryTerm : queryTerms) {
> try {
> subqs.add(ft.getFieldQuery(parser, sf, queryTerm));
> - } catch (Exception e) { // assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
> - // for edismax: ignore parsing failures
> + } catch (Exception e) {
> + /*
> + This happens when a field tries to parse a query term of incompatible type
> + e.g.
> + a numerical field trying to parse a textual query term
> + */
> + subqs.add(new MatchNoDocsQuery(queryTerm + " is not compatible with " + field));
>
> It appears this change (the addition of MatchNoDocsQuery here) has no
> effect but maybe I'm mistaken?
> ------------------------------
>
> In solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
> <https://github.com/apache/solr/pull/129#discussion_r638072636>:
>
> > return new RawQuery(sf, queryTerms);
> } else {
> if (queryTerms.size() == 1) {
> return ft.getFieldQuery(parser, sf, queryTerms.get(0));
> + } else if(ft instanceof StrField){
>
> In essence, I think the behavior I see here was correct *before* -- no
> special case for either StrField or numerics. In the context of the logic
> that reaches this point, the field is already ft.isTokenized==false.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/129#pullrequestreview-666761251>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAD5JK7YA7MGLQJO5BK2HM3TPJZUZANCNFSM444WCCXQ>
> .
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti edited a comment on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti edited a comment on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857805113
Making an update here to understand the potential next steps.
So I think we agree that:
1) if you configure a StrField in the QF and you configure sow=false, currently the parameter is ignored, you split by whitespace and you don't match your multi terms (it is like we use a query time tokenization, not in line with the expected invisible query time keyword tokenization of the StrField.) This is incorrect and gets you zero results when you shouldn't
2) the StrField instance of condition is not a solution you like, so we won't approve the pull request in the current state, N.B. I am completely fine with this. My objective here is to solve a problem, I am not in love with any specific solution.
So next steps:
3) it seems we want to "discourage" the usage of StrField in QF with an inconsistent behaviour (in comparison with a text field with the same keyword tokenization analysis). Should we then explicitly raise an error if the user passes a StrField (maybe all non-textual fields) in QF?
OR
4) should we remove the StrField type completely? we can leave the schema.xml back-compatible, but always use text fields for textual information server-side (I think it's the same approach that Elasticsearch uses and it makes sense to me)
OR
5) just ignore this issue? I am not a fan of this
OR
6) any other idea? super happy to explore other options
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644688429
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies ?
i.e.
StrField could be an alias rather than a separate Field Type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r635074347
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
+ String queryText = String.join(" ", queryTerms);
+ return ft.getFieldQuery(parser, sf, queryText);
} else {
List<Query> subqs = new ArrayList<>();
for (String queryTerm : queryTerms) {
try {
subqs.add(ft.getFieldQuery(parser, sf, queryTerm));
- } catch (Exception e) { // assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
- // for edismax: ignore parsing failures
+ } catch (Exception e) {
Review comment:
added a more detailed comment than the original, I don't know if the comment is comprehensive though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644688429
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-847194564
Given there's disagreement in changing this behaviour (is it bug or not?)
I suggest the following steps:
1) isolate the changes for the MM issue if possible in a separate Pull Request
2) find someone else opinion on both topics of this Pull Request ( the string field sow bug and the MM bug)
Should I tag any other people @dsmiley ?
I took the latest people involved in the class, but we didn't get any other opinion in the last 10 days
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: [SEARCH-15407] untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r632653580
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -412,24 +413,20 @@ public void testFocusQueryParser() {
nor);
// throw in a numeric field
- assertQ(req("defType","edismax", "mm","0", "q","Terminator: 100", "qf","movies_t foo_i"),
+ assertQ(req("defType","edismax", "mm","0", "q","Terminator: 100", "qf","movies_t foo_i","sow","true"),
twor);
assertQ(req("defType","edismax", "mm","100%", "q","Terminator: 100", "qf","movies_t foo_i", "sow","true"),
nor);
- // 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.
Review comment:
This comment basically describes my pending doubts, that I listed on Jira.
does mm=100% in a field centric approach means all compatible query terms in at least a field?
If I use sow=false and mm=100% a document just having the numerical part is returned and I am not sure is correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti edited a comment on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti edited a comment on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857896432
> iiuc there'd be no way to migrate current behavior for anyone who wanted to preserve it
Mmmm if you want to split on whitespace for a field in QF that is a StrField, you can just use sow=true.
I am not suggesting we should remove sow=true for StrFields, the objective here is to fix sow=false.
The reason I am saying that currently in my opinion we have a problem is that if you do:
Given q=<term1 term2>
**sow=true** -> StrField:term1 StrField:term2
**sow=false** -> StrField:term1 StrField:term2
As a user (my personal idea) if I set a StrFIeld in QF and set sow=false, I am relying on the query time tokenization approach of my QF fields (and query time tokenization approach for a StrField is "not to tokenize at all")
So if I have multi terms in the index for my StrField e.g. StrField: "term1 term2", currently I am unable to search for them both using sow=true or sow=false.
On the other hand if I have in my index [Doc1 StrField:"term1"] [Doc2 StrField:"term2"], for the query "term1 term2" I'll make sure to configure my sow=true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644684545
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853380214
> ** Current Incorrect Behaviour **
> If I query a "StrField" type or a text field type Keyword Tokenized, I get two different behaviours :
> sow=false -> it splits on whitespace anyway for StrField, causing incorrect results, for Keyword Tokenized texts, it works correctly
> sow=true -> to split on whitespace independently of the field type, it works correctly
This summary is helpful. If starting with a clean slate, perhaps the "correct" behavior should be as you've outlined -- but then I would think you'd want the same strict "non-splitting" behavior for integer fields. My gut reaction is that this is a very unusual use case (configuring a StrField or integer field as `qf`). In fact, a strong argument in principal for "strict" non-splitting behavior would be to effectively discourage this kind of configuration in the first place!
Ultimately though I think I'm converging on agreement with https://github.com/apache/solr/pull/129/files#r638071458 ... tbh I'm having a hard time imagining a use case where the configuration in question (or its current behavior) would be desirable; but assuming there may be such use cases already in place, this change could surely disrupt them.
Perhaps the fact that keyword-tokenized text fields currently work "correctly" (as defined above) could be employed to address this issue going forward for certain use-cases, rather than changing existing behavior? Keyword-tokenized text would also seem more appropriate to the `edismax` use-case, by providing analysis for text _normalization_ (the lack of which strikes me as the most unusual aspect of configuring StrField `qf`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r638118658
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,26 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
+ String queryText = String.join(" ", queryTerms);
+ return ft.getFieldQuery(parser, sf, queryText);
} else {
List<Query> subqs = new ArrayList<>();
for (String queryTerm : queryTerms) {
try {
subqs.add(ft.getFieldQuery(parser, sf, queryTerm));
- } catch (Exception e) { // assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
- // for edismax: ignore parsing failures
+ } catch (Exception e) {
+ /*
+ This happens when a field tries to parse a query term of incompatible type
+ e.g.
+ a numerical field trying to parse a textual query term
+ */
+ subqs.add(new MatchNoDocsQuery(queryTerm + " is not compatible with " + field));
Review comment:
The effect is in relation to the Minimum Should Match and sow, and the consequent bug detailed in thia Jira issue, it's not strictly related to the split on whitespace and strings, ideally two pull requests would be better, let me know and in case I can split the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r634849686
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Rather than hard-code this specific field type, maybe you should call `ft.isTokenized()==false`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r637963928
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
+ }
+
+ @Test
+ public void testSplitOnWhitespace_numericField_shouldBuildAlwaysMultiClause() throws Exception
Review comment:
Again, just drop "testSplitOnWhitespace_" from the method name, I think.
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,26 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
+ String queryText = String.join(" ", queryTerms);
+ return ft.getFieldQuery(parser, sf, queryText);
} else {
List<Query> subqs = new ArrayList<>();
for (String queryTerm : queryTerms) {
try {
subqs.add(ft.getFieldQuery(parser, sf, queryTerm));
- } catch (Exception e) { // assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
- // for edismax: ignore parsing failures
+ } catch (Exception e) {
+ /*
+ This happens when a field tries to parse a query term of incompatible type
+ e.g.
+ a numerical field trying to parse a textual query term
+ */
+ subqs.add(new MatchNoDocsQuery(queryTerm + " is not compatible with " + field));
Review comment:
It appears this change (the addition of MatchNoDocsQuery here) has no effect but maybe I'm mistaken?
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
Review comment:
Based on the test name, I'd expect sow=true each time. Maybe just drop this part of the method name.
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
In essence, I think the behavior I see here was correct *before* -- no special case for either StrField or numerics. In the context of the logic that reaches this point, the field is already ft.isTokenized==false.
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
Review comment:
This is a change in behavior, and I think it's not a good change. For a non-tokenized field (StrField in this case), I think we should ignore whatever "sow" is and split on whitespace any way, thus here have two terms to match. It would be straight-forward to document this (no differences between numbers and StrField).
I think it could be reasonable to try both ways (both split and don't split) and then put a DisjunctionMaxQuery over the two, though I'd prefer not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853836054
The core of my point is that that Solr *already* has an API to interrogate the field type to see if it produces one token or more -- {{FieldType.isTokenized}}. SolrQueryParserBase uses this in a number of places already. It appears you want to make StrField a further specialized case when isTokenized==false is not distinguishing enough and that appears to me going too far / hacky. It might be justifiable if we felt the result was clearly the correct behavior but we don't even think that (I don't, nor does Michael).
Nonetheless some user control could be added so that someone with a normalizing text field that uses TextField (likely with KeywordTokenizer but not necessarily) could get query parser behavior that is the same as how StrField behaves. It'd be nice if a user could set `tokenized="false"` on the fieldType definition that uses TextField if they can attest the processing doesn't tokenize. I experimented this morning with that and was able to do it by modifying the first line of TextField.init to be this snippet:
```
if ((falseProperties & TOKENIZED) == 0) {
properties |= TOKENIZED;
}
```
(instead of insisting TOKENIZED as it did before).
And maybe the existing logic in SolrQueryParserBase could be improved somehow; I'm definitely not claiming it's perfection. It just strikes me as a cast on StrField is a strong code smell to me that we're doing something wrong when there is already isTokenized which seems suitable for your purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-845620763
I'll look closer at this PR (e.g. in a debugger etc.) possibly tomorrow or if not then this weekend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-848134538
> isolate the changes for the MM issue if possible in a separate Pull Request
+1 indeed; if it fixes its own issue then it deserves a separate PR, especially for sensitive code like query parsers that will be noticed by users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644299691
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
Review comment:
disjunction `anyOf(...)` shouldn't be necessary to wrap a single clause -- can just use `containsString(...)` directly at the top level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti edited a comment on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti edited a comment on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-847790405
Thinking a little more about this I'll make even a bigger provocation:
I don't think it should exist a StrField type at all, it is textual information and we can get the same through a text field which is just "Keyword tokenized", and should be the only way, there's no benefit/differentiation in having two ways for the same index output.
But removing the StrField type is definitely not in scope in this issue, so in my opinion, we should at least have consistent behavior, and today if you define a field as StrField type or text Keyword tokenized, you produce the same index as far as I know, but at query time the sow works correctly just for the latter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857928192
I'm talking about preserving existing behavior for `sow=false` (the behavior that you -- and I, in fact -- are considering "incorrect" in principle). For better or worse, anyone who's currently using this config (if there is any such user) is using it with the current anomalous behavior of StrField and `sow=false`); so fixing this, even _if_ it's the "correct" thing to do, is likely to break any such configs "in the wild". In any event I think my postscript pseudocode-analysis-config probably obviates concern about there being no migration path to support existing behavior.
>As a user (my personal idea) ...
Understood! I don't require convincing that this is a problem, in principle. My personal expectation would be the same as yours, fwiw. All I'm suggesting (in considering a practical path forward) is to play the devil's advocate by sympathetically considering the alternate perspective; what use cases will be _broken_ by this change? I'm not even necessarily suggesting to _avoid_ breaking those use cases, but simply to be aware of them and proceed with caution, weighing the inertia of existing configs, and the oddness of configuring StrField `qf` under any circumstances.
I think either behavior is achievable under either implementation; so the question is whether the current behavior is objectively wrong enough to warrant a breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-847819259
From JIRA:
> it seems we get a document just containing "100" in the numerical field to be a match, even if it doesn't contain "terminator" anywhere else with mm=100%. And I think that's incorrect.
And I agreed. What test illustrates this problem? I'm suspecting the code _already_ works this way but I must be mistaken for you to pose this question in JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-845808565
that's much appreciated, I want a second pair of eyes minimum before merging this, so thanks David for your time.
I would be delighted to have an opinion also from the other committers that were recently involved in the edismax query parser : @madrob @romseygeek @munendrasn @sarowe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644298892
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
+ }
+
+ @Test
+ public void testQf_numericField_shouldBuildMultipleQueryClausesIndependentlyOfSow() throws Exception
+ {
+ assertJQ(req("qf", "foo_i", "defType", "edismax", "q", "101 102", "sow", "false"),
+ "/response/numFound==2", "/response/docs/[0]/id=='76'", "/response/docs/[1]/id=='77'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "foo_i", "q", "101 102", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("foo_i:[101 TO 101]"), containsString("foo_i:[102 TO 102]")));
Review comment:
I think you may want `allOf(...)` here (and line 1804 below) instead of `anyOf(...)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857896432
> iiuc there'd be no way to migrate current behavior for anyone who wanted to preserve it
Mmmm if you want to split on whitespace for a field in QF that is a StrField, you can just use sow=true .
The reason I am saying that currently in my opinion we have a problem is that if you do:
Given q=<term1 term2>
**sow=true** -> StrField:term1 StrField:term2
**sow=false** -> StrField:term1 StrField:term2
As a user (my personal idea) if I set a StrFIeld in QF and set sow=false, I am relying on the query time tokenization approach of my QF fields (and query time tokenization approach for a StrField is "not to tokenize at all")
So if I have multi terms in the index for my StrField e.g. StrField: "term1 term2", currently I am unable to search for them both using sow=true or sow=false.
On the other hand if I have in my index [Doc1 StrField:"term1"] [Doc2 StrField:"term2"], for the query "term1 term2" I'll make sure to configure my sow=true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644684545
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
Review comment:
Done!
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
+ }
+
+ @Test
+ public void testQf_numericField_shouldBuildMultipleQueryClausesIndependentlyOfSow() throws Exception
+ {
+ assertJQ(req("qf", "foo_i", "defType", "edismax", "q", "101 102", "sow", "false"),
+ "/response/numFound==2", "/response/docs/[0]/id=='76'", "/response/docs/[1]/id=='77'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "foo_i", "q", "101 102", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("foo_i:[101 TO 101]"), containsString("foo_i:[102 TO 102]")));
Review comment:
you are right, fixed!
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies ?
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies ?
i.e.
StrField could be an alias rather than a separate Field Type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644187229
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Adding a special case that references a specific field type is a code smell. Ideally, the FieldType API and/or interfaces express the logic to gate on here. There are already too many special cases in the existing code IMO. It seems to be already the case with ft.isTokenized(). I know the ultimate result is debatable but it's both-ways -- there's no obvious correct answer IMO. The differing behavior is something users are advised to steer clear away from, e.g. by using consistently analyzed fields in "qf".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r644685992
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1775,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testQf_stringFieldWithSowFalse_shouldBuildSingleQueryClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
+ }
+
+ @Test
+ public void testQf_numericField_shouldBuildMultipleQueryClausesIndependentlyOfSow() throws Exception
+ {
+ assertJQ(req("qf", "foo_i", "defType", "edismax", "q", "101 102", "sow", "false"),
+ "/response/numFound==2", "/response/docs/[0]/id=='76'", "/response/docs/[1]/id=='77'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "foo_i", "q", "101 102", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("foo_i:[101 TO 101]"), containsString("foo_i:[102 TO 102]")));
Review comment:
you are right, fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-842451631
the pull request has been updated with what I believe it is the current correct behavior:
/*
* sow=true implies the minimum should match is "per document"
* i.e a document to be a match must contain all the mm query terms anywhere at least once
* sow=false implies the minimum should match is "per field"
* i.e a document to be a match must contain all the mm query terms in a single field at least once
* */
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-852936901
Pull Request has been cleaned, only the fix related with the Jira issue is now present.
i'll summarize it again.
**Index Time**
Configuring a field to be "StrField" or "text" with a keyword tokenizer builds an inverted index with a single token per field value.
**Expected Query Time behaviour**
As a user using the edismax and typing a full text query I expect:
sow=false -> to not split on whitespace and delegate to the field text analysis (that for a StrField will not tokenize)
sow=true -> to split on whitespace independently of the field type
** Current Incorrect Behaviour **
If I query a "StrField" type or a text field type Keyword Tokenized, I get two different behaviours :
sow=false -> it splits on whitespace anyway for StrField, causing incorrect results, for Keyword Tokenized texts, it works correctly
sow=true -> to split on whitespace independently of the field type, it works correctly
The changes are clear in the tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti edited a comment on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti edited a comment on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857896432
> iiuc there'd be no way to migrate current behavior for anyone who wanted to preserve it
Mmmm if you want to split on whitespace for a field in QF that is a StrField, you can just use sow=true .
The reason I am saying that currently in my opinion we have a problem is that if you do:
Given q=<term1 term2>
**sow=true** -> StrField:term1 StrField:term2
**sow=false** -> StrField:term1 StrField:term2
As a user (my personal idea) if I set a StrFIeld in QF and set sow=false, I am relying on the query time tokenization approach of my QF fields (and query time tokenization approach for a StrField is "not to tokenize at all")
So if I have multi terms in the index for my StrField e.g. StrField: "term1 term2", currently I am unable to search for them both using sow=true or sow=false.
On the other hand if I have in my index [Doc1 StrField:"term1"] [Doc2 StrField:"term2"], for the query "term1 term2" I'll make sure to configure my sow=true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-847790405
Thinking a little more about this I'll make even a bigger provocation:
I don't think it should exist a StrField type at all, it is textual information and we can get the same through a text field which is just "Keyword tokenized", and should be the only way, there's no benefit/differentiation in having two ways for the same.
But removing the StrField type is definitely not in scope in this issue, so in my opinion, we should at least have consistent behavior, and today if you define a field as StrField type or text Keyword tokenized, the sow works correctly just for the latter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853779305
I agree it's not the most elegant solution to check on the type instance (but there's plenty in Solr that does that), but I still think that it is an inconsistency.
As specified in other comments, the root problem is the StrField type per se:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies?
i.e.
StrField could be an alias rather than a separate Field Type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r638116470
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
For numeric I agree with you, tokenization has little sense but for string fields, the won't be tokenized in the index.
Why would you like to have them tokenized at query time even if the user explicitly specified sow=false?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r635065459
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
Review comment:
Unfortunately ft.isTokenized() is false also for numericals.
And for numericals sow=false doesn't need to change anything.
It seems we need this behaviour with sow=false , only for StrField?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r634850011
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,18 +1151,21 @@ protected Query getFieldQuery(String field, List<String> queryTerms, boolean raw
return newFieldQuery
(getAnalyzer(), field, queryText, false, fieldAutoGenPhraseQueries, fieldEnableGraphQueries, synonymQueryStyle);
} else {
- if (raw) {
+ if (raw) {// assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
return new RawQuery(sf, queryTerms);
} else {
if (queryTerms.size() == 1) {
return ft.getFieldQuery(parser, sf, queryTerms.get(0));
+ } else if(ft instanceof StrField){
+ String queryText = String.join(" ", queryTerms);
+ return ft.getFieldQuery(parser, sf, queryText);
} else {
List<Query> subqs = new ArrayList<>();
for (String queryTerm : queryTerms) {
try {
subqs.add(ft.getFieldQuery(parser, sf, queryTerm));
- } catch (Exception e) { // assumption: raw = false only when called from ExtendedDismaxQueryParser.getQuery()
- // for edismax: ignore parsing failures
+ } catch (Exception e) {
Review comment:
I feel a comment is needed here. Do we know of particular exceptions where this happens (if so give an example in the comment) or is this more of a general safety fallback?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-853779305
I agree it's not the most elegant solution to check on the type instance (but there's plenty in Solr that does that), but I still think that it is an inconsistency.
As specified in other comments, the root problem is the StrField type per se:
Should we then start a much more complex task i.e. "removal of StrField" to avoid such inconsistencies?
We could try doing it in a back-compatible way, but ultimately I think the real issue is that we express text to be untokenized in the index in two ways with no apparent benefit but just inconsistencies?
i.e.
StrField could be an alias rather than a separate Field Type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-857888127
>This is incorrect and gets you zero results when you shouldn't
I'll acknowledge that the current behavior seems "incorrect" in principle to me (though just as incorrect for numeric fields). In terms of practical use cases, the current behavior strikes me as highly unusual -- but frankly no _more_ unusual than if the behavior were to be changed to respect `sow=false`. Perhaps I'm being closed-minded in thinking that what's "incorrect" is the config itself -- but before declaring the config to be canonically "incorrect" (and disallowing it), I think an open-minded attempt to brainstorm possible use cases would be in order.
That's particularly the case because:
1. The change in behavior would be dramatic (for anyone actually relying on this config); it's also worth noting that the current behavior is not newly introduced, and I'm not aware of any instances of users expressing confusion about it in the past (again to be clear, my initial inclination _is_ to consider `qf` against non-analyzed fields to be "incorrect" in principle)
2. iiuc there'd be no way to migrate current behavior for anyone who wanted to preserve it
3. There's currently a viable workaround (and a more "search-idiomatic" one at that, via KeywordTokenizer) for anyone who desires the proposed "correct" behavior.
Hmm... on second thought maybe wrt point 2:
```xml
<analyzer type="index">
[KeywordTokenizer]
</analyzer>
<analyzer type="query">
[WhitespaceTokenizer]
...
</analyzer>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a change in pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on a change in pull request #129:
URL: https://github.com/apache/solr/pull/129#discussion_r638121503
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
Review comment:
changed the name, any better?
##########
File path: solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -1771,6 +1787,35 @@ public void testSplitOnWhitespace_Basic() throws Exception {
assertThat(parsedquery, anyOf(containsString("((name:stigma | title:stigma))"), containsString("((title:stigma | name:stigma))")));
}
+ @Test
+ public void testSplitOnWhitespace_stringField_shouldBuildSingleClause() throws Exception
+ {
+ assertJQ(req("qf", "trait_ss", "defType", "edismax", "q", "multi term", "sow", "false"),
+ "/response/numFound==1", "/response/docs/[0]/id=='75'");
+
+ String parsedquery = getParsedQuery(
+ req("qf", "trait_ss", "q", "multi term", "defType", "edismax", "sow", "false", "debugQuery", "true"));
+ assertThat(parsedquery, anyOf(containsString("((trait_ss:multi term))")));
+ }
+
+ @Test
+ public void testSplitOnWhitespace_numericField_shouldBuildAlwaysMultiClause() throws Exception
Review comment:
changed the name, any better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #129: SOLR-15407 untokenized field type with sow=false fix + tests
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #129:
URL: https://github.com/apache/solr/pull/129#issuecomment-842451631
the pull request has been updated with what I believe it is the current correct behavior:
/*
* sow=true implies the minimum should match is "per document"
* i.e a document to be a match must contain all the mm query terms anywhere at least once
* sow=false implies the minimum should match is "per field"
* i.e a document to be a match must contain all the mm query terms in a single field at least once
* */
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org