[
https://issues.apache.org/jira/browse/SOLR-11865?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16395483#comment-16395483
]
David Smiley commented on SOLR-11865:
-------------------------------------
Thanks Bruno.
It seems there is _some_ new/changed behavior (and that's fine):
* can define the same query more than once and it'll get merged; no longer an
exception
* new {{keepElevationPriority}} parameter
Comments:
* I'm a little skeptical about the need for InitializationExceptionHandler,
LoadingExceptionHandler, and related complexities. Maybe you can provide some
insight as to why this is needed?
* IMO it's unfortunate that ElevationProvider is mutable and has the
makeImmutable method. How about createElevationProvider accept the
elevationBuilderMap? And does ElevationProvider.size need to be there either?
And does getElevationForQuery need to throw an IOException? With such
simplifications, we could then simply use JDK Function<String,Elevation>. Not
that using the JDK one is a big deal (and it's debatable too) but my point is
more about simplifying.
* the indentation around line ~671 (contents of the for loop) is messed up; it
made me misinterpret the intent of the the logic
* My preference is to not have javadoc comments like "<p>Can be overridden by
extending this class.</p>" because "protected" access implies this, thus it's
redundant.
* suggest change comparator docVal (~line 1318) to use getOrDefault
* suggest to use {{localBoosts.addAll(boosted.keySet());}} at line ~661 instead
of manual looping (IntelliJ helpfully pointed this out)
* in parseExcludedMarkerFieldName and parseEditorialMarkerFieldName I see
initArgs.get being called with a default value, yet it's followed up with a
check for null to then use the default value. This seems quite redundant since
the two-arg SolrParams.get already does that. I don't like the empty string
check (this is for a config file, not a request parameter where a better
argument could be made for it) -- I'd much prefer something tighter/simpler.
It appears SOLR-2037 introduced this but it was a minor detail that wasn't
discussed in the comments. Removing this would technically be a back-compat
break but it's minor enough and so easy for someone to fix that I think it's
fine.
* Instead of IndexedValueProvider, which we don't even expose, lets just use a
UnaryOperator<String>? and then use a Java 8 method reference when needed
instead of declaring QueryElevationComponent.indexedValueProvider ?
* Maybe make the constructor of ElevatingQuery protected so it could be
subclassed. Likewise for Elevation.
BTW this code pattern {{seen.contains(id) == false}} is often written as-such
deliberately instead of {{!seen.contains(id)}} because it reads clearer (and
takes no additional lines of code). Bugs have been missed then found because
of that style. There is no code standard for it in Lucene or Solr but I
recommend against modifying existing lines that made one choice or the other as
part this cleanup.
> Refactor QueryElevationComponent to prepare query subset matching
> -----------------------------------------------------------------
>
> Key: SOLR-11865
> URL: https://issues.apache.org/jira/browse/SOLR-11865
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: SearchComponents - other
> Affects Versions: master (8.0)
> Reporter: Bruno Roustant
> Priority: Minor
> Labels: QueryComponent
> Fix For: master (8.0)
>
> Attachments:
> 0001-Refactor-QueryElevationComponent-to-introduce-Elevat.patch,
> SOLR-11865.patch
>
>
> The goal is to prepare a second improvement to support query terms subset
> matching or query elevation rules.
> Before that, we need to refactor the QueryElevationComponent. We make it
> extendible. We introduce the ElevationProvider interface which will be
> implemented later in a second patch to support subset matching. The current
> full-query match policy becomes a default simple MapElevationProvider.
> - Add overridable methods to handle exceptions during the component
> initialization.
> - Add overridable methods to provide the default values for config properties.
> - No functional change beyond refactoring.
> - Adapt unit test.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
