[jira] [Commented] (SOLR-11865) Refactor QueryElevationComponent to prepare query subset matching

["David Smiley (JIRA)" <ji...@apache.org>]

    [ 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org