magibney commented on a change in pull request #158:
URL: https://github.com/apache/solr/pull/158#discussion_r644332142
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,7 +1150,7 @@ 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()
Review comment:
I wonder whether it makes more sense to leave this "assumption" comment
where it was -- at the point that the exception is swallowed? No doubt the
comment is _true_ either place, but placing the comment up here separates it
from where action is taken _based_ on that assumption.
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1159,8 +1160,11 @@ protected Query getFieldQuery(String field, List<String>
queryTerms, boolean raw
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 that
has a type incompatible with the field
+ // e.g.
+ // a numerical field trying to parse a textual query term
+ subqs.add(new MatchNoDocsQuery(queryTerm + " is not compatible
with " + field));
Review comment:
Minor point, but I think it'd be fine to be less specific in the
"reason" arg to the MatchNoDocsQuery. Not to micro-optimize, but the
specificity is unlikely to _gain_ anything here practically speaking (right?).
(A cursory check for existing MatchNoDocsQuery constructor invocations in Solr
and Lucene shows that all seem to either invoke the no-arg ctor or pass in a
static String)
##########
File path:
solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java
##########
@@ -415,19 +415,7 @@ public void testFocusQueryParser() {
assertQ(req("defType","edismax", "mm","0", "q","Terminator: 100",
"qf","movies_t foo_i"),
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:
@sarowe, the above comment (from
dd171ff8fe31df578b7e6fab1eb5bfc1ade3f5fc) 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.
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,7 +1150,7 @@ 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()
Review comment:
Yeah, either way's fine. I meant that we're "swallowing" the exception
in the sense that we're not _rethrowing_ it, which lenient approach is
warranted (iiuc) by the assumption that this code is only called from from the
lenient edismax parser. I guess my thought was: if I'm a developer looking at
the exception get caught and not rethrown, I'm likely to wonder "why be lenient
and not rethrow? -- ah! because of this commented assumption!". By contrast, if
I'm a developer looking at the {{if (raw) {}} line, I'll see the comment and
wonder "why do I care about where I'm being called from at this point?".
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1149,7 +1150,7 @@ 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()
Review comment:
Yeah, either way's fine. I meant that we're "swallowing" the exception
in the sense that we're not _rethrowing_ it, which lenient approach is
warranted (iiuc) by the assumption that this code is only called from from the
lenient edismax parser. I guess my thought was: if I'm a developer looking at
the exception get caught and not rethrown, I'm likely to wonder "why be lenient
and not rethrow? -- ah! because of this commented assumption!". By contrast, if
I'm a developer looking at the `if (raw) {` line, I'll see the comment and
wonder "why do I care about where I'm being called from at this point?".
##########
File path: solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java
##########
@@ -1159,8 +1160,11 @@ protected Query getFieldQuery(String field, List<String>
queryTerms, boolean raw
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 that
has a type incompatible with the field
+ // e.g.
+ // a numerical field trying to parse a textual query term
+ subqs.add(new MatchNoDocsQuery(queryTerm + " is not compatible
with " + field));
Review comment:
Thanks, sure; or a static "reason" String, e.g. "field-incompatible
clause should count as no match" -- in any case, point being to avoid dynamic
creation of a String that'll likely be ignored anyway.
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
