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 2022/03/23 16:26:29 UTC
[GitHub] [solr] cpoerschke opened a new pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
cpoerschke opened a new pull request #757:
URL: https://github.com/apache/solr/pull/757
https://issues.apache.org/jira/browse/SOLR-16111
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r838479555
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,27 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ final Set<String> fields =
+ Set.of(expandWildcardsInHighlightFields(indexedFields, queryFieldPattern));
+ if (fields.isEmpty()) {
+ return NO_FIELD_MATCH_PREDICATE;
+ }
Review comment:
Fixed in https://github.com/apache/solr/pull/757/commits/0ac02d374fa95ac590c6e83d7b95642b1f813641 commit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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 #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837682433
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,27 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ final Set<String> fields =
+ Set.of(expandWildcardsInHighlightFields(indexedFields, queryFieldPattern));
+ if (fields.isEmpty()) {
+ return NO_FIELD_MATCH_PREDICATE;
+ }
Review comment:
These lines are fine but I just want to point out that it isn't necessary and it's a dubious optimization.
##########
File path: solr/solr-ref-guide/modules/query-guide/pages/highlighting.adoc
##########
@@ -116,6 +116,17 @@ If set to `true`, only query terms aligning with the field being highlighted wil
If the query references fields different from the field being highlighted and they have different text analysis, the query may not highlight query terms it should have and vice versa.
The analysis used is that of the field being highlighted (`hl.fl`), not the query fields.
+`hl.queryFieldPattern`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `none`
+|===
++
+Similar to `hl.requireFieldMatch` but allows for multiple fields to match e.g. `q=fieldA:one OR fieldB:two OR fieldC:three` `hl.fl=fieldA` `hl.queryFieldPattern=fieldA,fieldB`
Review comment:
Then please say so in the docs; that's what I was hinting at. We could also conceivably throw a request error if both are specified, which would be nice but I leave it to you if you wish to.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r838479132
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
Review comment:
Done in https://github.com/apache/solr/pull/757/commits/f4e949e683d60652daeb0e9333cb5fce5a196553 commit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r834052452
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,35 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> storedHighlightFieldNames =
+ solrIndexSearcher.getDocFetcher().getStoredHighlightFieldNames();
Review comment:
Actually "indexed" rather than "stored" is the appropriate criteria here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #757:
URL: https://github.com/apache/solr/pull/757#issuecomment-1081890148
Converting to draft until #767 is merged first and then this pull request updated to account for the signature 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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837695857
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,27 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ final Set<String> fields =
+ Set.of(expandWildcardsInHighlightFields(indexedFields, queryFieldPattern));
+ if (fields.isEmpty()) {
+ return NO_FIELD_MATCH_PREDICATE;
+ }
Review comment:
Ah, yes, structural carry-over from the `if (predicate == null) {` that was there earlier but empty set would handle it implicitly, good spot.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837367923
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
+ for (FieldInfo fieldInfo : searcher.getFieldInfos()) {
+ final String fieldName = fieldInfo.name;
+ try {
+ SchemaField field = searcher.getSchema().getField(fieldName);
Review comment:
Both `getStoredHighlightFieldNames()` above and the logic in the `SolrDocumentFetcher` are consulting Solr's schema. Though they differ in terms of 'error' handling e.g. the method logs a warning whereas the constructor silently skips.
`fieldInfo.getIndexOptions() != IndexOptions.NONE` seems to be the `FieldInfo` equivalent, will give that a go.
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
Review comment:
Was modelling this on `getStoredHighlightFieldNames()` above, any thoughts on changing that one also to `ArrayList` at the same time?
##########
File path: solr/solr-ref-guide/modules/query-guide/pages/highlighting.adoc
##########
@@ -116,6 +116,17 @@ If set to `true`, only query terms aligning with the field being highlighted wil
If the query references fields different from the field being highlighted and they have different text analysis, the query may not highlight query terms it should have and vice versa.
The analysis used is that of the field being highlighted (`hl.fl`), not the query fields.
+`hl.queryFieldPattern`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `none`
+|===
++
+Similar to `hl.requireFieldMatch` but allows for multiple fields to match e.g. `q=fieldA:one OR fieldB:two OR fieldC:three` `hl.fl=fieldA` `hl.queryFieldPattern=fieldA,fieldB`
Review comment:
No, if both `hl.requireFieldMatch` and `hl.queryFieldPattern` are specified then only the former one is used and the latter is (silently) ignored.
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,33 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ Predicate<String> predicate = null;
+ for (String f : expandWildcardsInHighlightFields(indexedFields, queryFieldPattern)) {
+ if (predicate == null) {
+ predicate = f::equals;
+ } else {
+ predicate = predicate.or(f::equals);
+ }
Review comment:
Good point, thanks! Will do.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r838478595
##########
File path: solr/solr-ref-guide/modules/query-guide/pages/highlighting.adoc
##########
@@ -116,6 +116,17 @@ If set to `true`, only query terms aligning with the field being highlighted wil
If the query references fields different from the field being highlighted and they have different text analysis, the query may not highlight query terms it should have and vice versa.
The analysis used is that of the field being highlighted (`hl.fl`), not the query fields.
+`hl.queryFieldPattern`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `none`
+|===
++
+Similar to `hl.requireFieldMatch` but allows for multiple fields to match e.g. `q=fieldA:one OR fieldB:two OR fieldC:three` `hl.fl=fieldA` `hl.queryFieldPattern=fieldA,fieldB`
Review comment:
docs updated in https://github.com/apache/solr/pull/757/commits/ee6f1e3e15457d8e89deaf5e68abba199568e586 and also in https://github.com/apache/solr/pull/757/commits/44bd2201f834dd7ef35773e1e3630cf5838ca2b2 added test for it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837492657
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,33 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ Predicate<String> predicate = null;
+ for (String f : expandWildcardsInHighlightFields(indexedFields, queryFieldPattern)) {
+ if (predicate == null) {
+ predicate = f::equals;
+ } else {
+ predicate = predicate.or(f::equals);
+ }
Review comment:
Changed in https://github.com/apache/solr/pull/757/commits/00e3ccdd9485c6e3627d95dc0c0321ff248a0766 commit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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 #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r836876499
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
+ for (FieldInfo fieldInfo : searcher.getFieldInfos()) {
+ final String fieldName = fieldInfo.name;
+ try {
+ SchemaField field = searcher.getSchema().getField(fieldName);
Review comment:
This isn't necessary; can't we look at FieldInfo for this without consulting Solr's schema?
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
Review comment:
Let's use an ArrayList instead
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,33 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ Predicate<String> predicate = null;
+ for (String f : expandWildcardsInHighlightFields(indexedFields, queryFieldPattern)) {
+ if (predicate == null) {
+ predicate = f::equals;
+ } else {
+ predicate = predicate.or(f::equals);
+ }
Review comment:
this results in a O(N) predicate. I think it would be more efficient to simply build up a HashSet and have a simple predicate to check it -- O(1).
##########
File path: solr/solr-ref-guide/modules/query-guide/pages/highlighting.adoc
##########
@@ -116,6 +116,17 @@ If set to `true`, only query terms aligning with the field being highlighted wil
If the query references fields different from the field being highlighted and they have different text analysis, the query may not highlight query terms it should have and vice versa.
The analysis used is that of the field being highlighted (`hl.fl`), not the query fields.
+`hl.queryFieldPattern`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `none`
+|===
++
+Similar to `hl.requireFieldMatch` but allows for multiple fields to match e.g. `q=fieldA:one OR fieldB:two OR fieldC:three` `hl.fl=fieldA` `hl.queryFieldPattern=fieldA,fieldB`
Review comment:
Does this supersede hl.requireFieldMatch when set?
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,33 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ Predicate<String> predicate = null;
+ for (String f : expandWildcardsInHighlightFields(indexedFields, queryFieldPattern)) {
Review comment:
You created a expandWildcardsInHighlightFields method whose first argument has a name storedHighlightFieldNames. But at this line you pass indexed fields. This is kind of suspicious.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r834574449
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,35 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> storedHighlightFieldNames =
+ solrIndexSearcher.getDocFetcher().getStoredHighlightFieldNames();
Review comment:
changed in https://github.com/apache/solr/pull/757/commits/16fec2b5a9663350aee74c938679940cfc79daee
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837306552
##########
File path: solr/core/src/java/org/apache/solr/highlight/UnifiedSolrHighlighter.java
##########
@@ -431,14 +435,33 @@ protected Locale parseLocale(String language, String country, String variant) {
@Override
protected Predicate<String> getFieldMatcher(String field) {
- // TODO define hl.queryFieldPattern as a more advanced alternative to hl.requireFieldMatch.
// note that the UH at Lucene level default to effectively "true"
if (params.getFieldBool(field, HighlightParams.FIELD_MATCH, false)) {
return field::equals; // requireFieldMatch
- } else {
- return NOT_REQUIRED_FIELD_MATCH_PREDICATE;
}
+
+ String[] queryFieldPattern =
+ params.getFieldParams(field, HighlightParams.QUERY_FIELD_PATTERN);
+ if (queryFieldPattern != null && queryFieldPattern.length != 0) {
+
+ Collection<String> indexedFields = solrIndexSearcher.getDocFetcher().getIndexedFieldNames();
+
+ Predicate<String> predicate = null;
+ for (String f : expandWildcardsInHighlightFields(indexedFields, queryFieldPattern)) {
Review comment:
Fair point. https://github.com/apache/solr/pull/767/commits/d48a4a623ef5f50f13315314690bad9652b154be on #767 to drop the "stored" and "highlight" from the argument name (and to add "supplier") plus also to omit "highlight" from the method name since actually there's nothing highlighting specific about the expansion and the class name already indicates highlighting scope.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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] cpoerschke commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837492001
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
+ for (FieldInfo fieldInfo : searcher.getFieldInfos()) {
+ final String fieldName = fieldInfo.name;
+ try {
+ SchemaField field = searcher.getSchema().getField(fieldName);
Review comment:
Changed in https://github.com/apache/solr/pull/757/commits/58abfdd6557700585b8abfa9c8ef698e185d74f6 commit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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 #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #757:
URL: https://github.com/apache/solr/pull/757#discussion_r837678822
##########
File path: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java
##########
@@ -219,6 +221,28 @@ public boolean isLazyFieldLoadingEnabled() {
}
}
+ /** Returns a collection of the names of all indexed fields which the index reader knows about. */
+ public Collection<String> getIndexedFieldNames() {
+ synchronized (this) {
+ if (indexedFieldNames == null) {
+ indexedFieldNames = new LinkedList<>();
Review comment:
Yeah I figured you took the same algorithm, hence the LinkedList. Yes please change to ArrayList there too If you don't mind.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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