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/28 21:55:01 UTC
[GitHub] [solr] dsmiley commented on a change in pull request #757: SOLR-16111: hl.queryFieldPattern support (advanced alternative to hl.requireFieldMatch)
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