Solr
  1. Solr
  2. SOLR-2037

Distinguish Editorial Results from "normal" results in the QueryElevationComponent

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      In many cases, it would be nice if the results that are provided by the QueryElevationComponent were identified as such so that one could make a decision at rendering time whether to treat them differently or not.

      1. SOLR-2037.patch
        19 kB
        Grant Ingersoll
      2. SOLR-2037.patch
        18 kB
        Grant Ingersoll
      3. SOLR-2037.patch
        17 kB
        Grant Ingersoll
      4. EditorialMarkerFactory.java
        2 kB
        Ryan McKinley
      5. SOLR-2037-editoral-marker.patch
        4 kB
        Ryan McKinley

        Issue Links

          Activity

          Hide
          Grant Ingersoll added a comment -

          Now that SOLR-1566 is fixed, we should be able to do this. The main issue is this gets hooked into sorting, so we don't really have access to the documents to mark them. Now, I could still hook it into the response writer as a transformer that uses the elevation info, but that isn't surefire either since we wouldn't truly know if it was elevated or not. The other option, perhaps is that the FieldcomparatorSource used would need to put something into a data structure in the request that can then be consumed by a Transformer later. Not sure this will work w/ the current setup, as it caches the comparator. To deal with that, might need to separate that part out a little bit so as to be able to construct a really lightweight comparator that is constructed each time, but wraps an object that has the main sorting configuration done only when needed per the current approach.

          Show
          Grant Ingersoll added a comment - Now that SOLR-1566 is fixed, we should be able to do this. The main issue is this gets hooked into sorting, so we don't really have access to the documents to mark them. Now, I could still hook it into the response writer as a transformer that uses the elevation info, but that isn't surefire either since we wouldn't truly know if it was elevated or not. The other option, perhaps is that the FieldcomparatorSource used would need to put something into a data structure in the request that can then be consumed by a Transformer later. Not sure this will work w/ the current setup, as it caches the comparator. To deal with that, might need to separate that part out a little bit so as to be able to construct a really lightweight comparator that is constructed each time, but wraps an object that has the main sorting configuration done only when needed per the current approach.
          Hide
          Ryan McKinley added a comment -

          this is a super quick/dirty sketch on how you may be able to do it just by sticking something into the request context.

          To do this for real, we would need to do somethign smarter with the ids

          Show
          Ryan McKinley added a comment - this is a super quick/dirty sketch on how you may be able to do it just by sticking something into the request context. To do this for real, we would need to do somethign smarter with the ids
          Hide
          Grant Ingersoll added a comment -

          Ah, makes sense, Ryan. I forgot that we have the ids handy outside of the comparator. I'll add some tests, etc.

          Show
          Grant Ingersoll added a comment - Ah, makes sense, Ryan. I forgot that we have the ids handy outside of the comparator. I'll add some tests, etc.
          Hide
          Grant Ingersoll added a comment -

          Hmm, Ryan. Not sure this is going to work. The creation of the factory happens before the prepare method gets called, so the BOOSTED value is always null, AFAICT.

          Show
          Grant Ingersoll added a comment - Hmm, Ryan. Not sure this is going to work. The creation of the factory happens before the prepare method gets called, so the BOOSTED value is always null, AFAICT.
          Hide
          Yonik Seeley added a comment - - edited

          As far as interface... what about by default adding an _elevated_ pseudo-field?

          Show
          Yonik Seeley added a comment - - edited As far as interface... what about by default adding an _elevated_ pseudo-field?
          Hide
          Ryan McKinley added a comment -

          ah right – what about lazy loading the BOOSTED set?

          This is a bit more ugly, but could work
          (again, totally untested... just here for suggestion)

          Show
          Ryan McKinley added a comment - ah right – what about lazy loading the BOOSTED set? This is a bit more ugly, but could work (again, totally untested... just here for suggestion)
          Hide
          Grant Ingersoll added a comment -

          Seems like transform() should just take in the Context.

          Show
          Grant Ingersoll added a comment - Seems like transform() should just take in the Context.
          Hide
          Ryan McKinley added a comment -

          ya, that would also work – TextResponseWriter has the request.

          Show
          Ryan McKinley added a comment - ya, that would also work – TextResponseWriter has the request.
          Hide
          Grant Ingersoll added a comment -

          Here's a patch that adds the SolrQueryRequest to the transform call. Not sure yet what the implications of that are for the TransformerContext. Seems like we aught to just use the Request.getContext() instead of the setContext stuff. Simpler and one less data structure to manage.

          Show
          Grant Ingersoll added a comment - Here's a patch that adds the SolrQueryRequest to the transform call. Not sure yet what the implications of that are for the TransformerContext. Seems like we aught to just use the Request.getContext() instead of the setContext stuff. Simpler and one less data structure to manage.
          Hide
          Grant Ingersoll added a comment -

          I should add the patch also has tests that now pass for using the EditorialMarkerFactory

          Show
          Grant Ingersoll added a comment - I should add the patch also has tests that now pass for using the EditorialMarkerFactory
          Hide
          Yonik Seeley added a comment -

          Not sure yet what the implications of that are for the TransformerContext.

          Something like that will still be needed in the future to handle transformers that need to do stuff in batches and not doc-at-a-time.
          One example of this is function queries containing a relevancy query component - those aren't random access.

          As far as naming, [elevated]=true seems more readable than [qecBooster]=true?

          Show
          Yonik Seeley added a comment - Not sure yet what the implications of that are for the TransformerContext. Something like that will still be needed in the future to handle transformers that need to do stuff in batches and not doc-at-a-time. One example of this is function queries containing a relevancy query component - those aren't random access. As far as naming, [elevated] =true seems more readable than [qecBooster] =true?
          Hide
          Grant Ingersoll added a comment -

          As far as naming, [elevated]=true seems more readable than [qecBooster]=true?

          Yeah, I can change that (I'd like to change the name of the QEC too). I'm on the fence as to whether this one should be on by default or not, since the QEC is not on by default.

          Show
          Grant Ingersoll added a comment - As far as naming, [elevated] =true seems more readable than [qecBooster] =true? Yeah, I can change that (I'd like to change the name of the QEC too). I'm on the fence as to whether this one should be on by default or not, since the QEC is not on by default.
          Hide
          Ryan McKinley added a comment -

          TransformerContext != Request.getContext() it has lots of info about the query that made the DocList – this is necessary for things that need to know the actual Query and DocIterator – see ExplainAugmenterFactory.

          Rather then adding SolrQueryRequest to the transform, we could also add it to the TransformerContext.

          Yes, setContext() is potentially more error prone then passing it along with every transform call – but this also gives us a consistent way to avoid lazy loading. We know setContext is called before any documents are transformed. Rather then have the lazy load logic i suggested earler, what about putting that in setContext?

          Show
          Ryan McKinley added a comment - TransformerContext != Request.getContext() it has lots of info about the query that made the DocList – this is necessary for things that need to know the actual Query and DocIterator – see ExplainAugmenterFactory. Rather then adding SolrQueryRequest to the transform, we could also add it to the TransformerContext. Yes, setContext() is potentially more error prone then passing it along with every transform call – but this also gives us a consistent way to avoid lazy loading. We know setContext is called before any documents are transformed. Rather then have the lazy load logic i suggested earler, what about putting that in setContext?
          Hide
          Grant Ingersoll added a comment -

          Switches to elevated, handles the case where elevated is requested, but no items are elevated.

          Show
          Grant Ingersoll added a comment - Switches to elevated, handles the case where elevated is requested, but no items are elevated.
          Hide
          Yonik Seeley added a comment -

          I'm on the fence as to whether this one should be on by default or not, since the QEC is not on by default.

          Right, we shouldn't be adding [elevated]=false to all the docs by default, even when QEC isn't being used.

          The only question left then is if the augmenter/transformer should be on by default if QEC is being used (i.e. forceElevation==true?) Or should the user explicitly put [elevated] in their field list? I'm thinking the latter, for folks who put fl=id and expect only the id field to be returned.

          It would also be nice if one didn't have to define this EditorialMarkerFactory in solr.xml... seems like from the user perspective it should "just work" (either the QEC can dynamically register it, or it can always be registered by default like explain, docid, etc).

          Show
          Yonik Seeley added a comment - I'm on the fence as to whether this one should be on by default or not, since the QEC is not on by default. Right, we shouldn't be adding [elevated] =false to all the docs by default, even when QEC isn't being used. The only question left then is if the augmenter/transformer should be on by default if QEC is being used (i.e. forceElevation==true?) Or should the user explicitly put [elevated] in their field list? I'm thinking the latter, for folks who put fl=id and expect only the id field to be returned. It would also be nice if one didn't have to define this EditorialMarkerFactory in solr.xml... seems like from the user perspective it should "just work" (either the QEC can dynamically register it, or it can always be registered by default like explain, docid, etc).
          Hide
          Grant Ingersoll added a comment -

          @Ryan, that clarifies things. I have left it as is.

          @Yonik:

          The only question left then is if the augmenter/transformer should be on by default if QEC is being used (i.e. forceElevation==true?) Or should the user explicitly put [elevated] in their field list? I'm thinking the latter, for folks who put fl=id and expect only the id field to be returned.

          Agreed, I think it should be off by default.

          It would also be nice if one didn't have to define this EditorialMarkerFactory in solr.xml... seems like from the user perspective it should "just work" (either the QEC can dynamically register it, or it can always be registered by default like explain, docid, etc).

          Good point. Consider it done.

          Show
          Grant Ingersoll added a comment - @Ryan, that clarifies things. I have left it as is. @Yonik: The only question left then is if the augmenter/transformer should be on by default if QEC is being used (i.e. forceElevation==true?) Or should the user explicitly put [elevated] in their field list? I'm thinking the latter, for folks who put fl=id and expect only the id field to be returned. Agreed, I think it should be off by default. It would also be nice if one didn't have to define this EditorialMarkerFactory in solr.xml... seems like from the user perspective it should "just work" (either the QEC can dynamically register it, or it can always be registered by default like explain, docid, etc). Good point. Consider it done.
          Hide
          Ryan McKinley added a comment -

          rather then:

          -  public abstract void transform(SolrDocument doc, int docid) throws IOException;
          +  public abstract void transform(SolrDocument doc, int docid, SolrQueryRequest req) throws IOException;
          

          What about adding SolrQueryRequest to the TransformerContext? This could avoid the lazy load logic in this Transformer, and also give access to SolrQueryRequest for things that need batch processing (as yonik discussed)

          Or should the user explicitly put [elevated] in their field list?

          +1 this should all be explicit

          Show
          Ryan McKinley added a comment - rather then: - public abstract void transform(SolrDocument doc, int docid) throws IOException; + public abstract void transform(SolrDocument doc, int docid, SolrQueryRequest req) throws IOException; What about adding SolrQueryRequest to the TransformerContext? This could avoid the lazy load logic in this Transformer, and also give access to SolrQueryRequest for things that need batch processing (as yonik discussed) Or should the user explicitly put [elevated] in their field list? +1 this should all be explicit
          Hide
          Grant Ingersoll added a comment -

          Auto-registers the Marker Factory, but still allows the name to be picked by the user, if they so choose.

          Switches to using the TransformContext.

          All tests pass. I believe it should be ready to go.

          Show
          Grant Ingersoll added a comment - Auto-registers the Marker Factory, but still allows the name to be picked by the user, if they so choose. Switches to using the TransformContext. All tests pass. I believe it should be ready to go.
          Hide
          Ryan McKinley added a comment -

          +1

          not sure what (if anythign) belongs in solrconfig.xml

          After this goes in, I will add some more javadocs to tranformer stuff

          Show
          Ryan McKinley added a comment - +1 not sure what (if anythign) belongs in solrconfig.xml After this goes in, I will add some more javadocs to tranformer stuff
          Hide
          Grant Ingersoll added a comment -

          not sure what (if anythign) belongs in solrconfig.xml

          You can, optionally, in the QEC configuration specify:
          {{

          { <str name="editorialMarkerFieldName">foo</str> }

          }}

          and that will change the name of the pseudo-field from "elevated" to whatever specified. This way, if for some reason someone already has elevated it won't be confused. Of course, it's really "[elevated]" anyway, but still less confusion, hopefully.

          Show
          Grant Ingersoll added a comment - not sure what (if anythign) belongs in solrconfig.xml You can, optionally, in the QEC configuration specify: {{ { <str name="editorialMarkerFieldName">foo</str> } }} and that will change the name of the pseudo-field from "elevated" to whatever specified. This way, if for some reason someone already has elevated it won't be confused. Of course, it's really " [elevated] " anyway, but still less confusion, hopefully.
          Hide
          Grant Ingersoll added a comment -

          Committed. Of course, the next thing I want is to know what the old rank was, but that is likely much harder.

          Show
          Grant Ingersoll added a comment - Committed. Of course, the next thing I want is to know what the old rank was, but that is likely much harder.

            People

            • Assignee:
              Grant Ingersoll
              Reporter:
              Grant Ingersoll
            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development