[jira] [Updated] (SOLR-4212) Let facet queries hang off of pivots

["Shalin Shekhar Mangar (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SOLR-4212?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Shalin Shekhar Mangar updated SOLR-4212:
----------------------------------------
    Attachment: SOLR-6353-6686-4212.patch

Thanks for the detailed review, Hoss.

bq. verbose: why not just "ranges" and "queries" for the local param names, similar to how "stats" is already used?

That was an oversight. Fixed.

bq. I'm not a big fan of the fact that this patch breaks the determinism of the order that the different types of facets are returned in – It's probably not a blocker, but i suspect there may be some clients (or perhaps even client libraries other then SolrJ) which will be broken by this

Fixed.

bq. In the SolrJ PivotField class, this patch adds "NamedList<Object> getRanges()" and "NamedList<Integer> getQueryCounts()" methods. We should really be consistent with the existing equivalent methods in QueryResponse

Fixed.

bq. why can't we just use "Number" instead of "Object" in this new code since that's the lavel all of the casting code that deals with this list seems to use anyway?

Done.

bq. doesn't this break the SolrJ code if/when it returns a Long? (see above new method "NamedList<Integer> getQueryCounts()")

Yes it does. And the current SolrJ code for range & query facets also needs to be fixed. I'll open another issue to fix the client side of things.

bq. DateFacetProcessor -- this entire class should be marked deprecated

Done.

bq. maybe i'm missing something, but what exactly is the advantage of this subclassing RangeFacetProcessor? ... if they are sharing code it's not obvious to me, and if they aren't (intentionally) sharing code then this subclass relationship seems dangerous if/when future improvements to range faceting are made.

This was an oversight. Fixed.

bq. FacetComponent -- why does doDistribRanges() still need to exist? why not just move that casting logic directly into RangeFacetAccumulator.mergeContributionFromShard like the rest of the code that use to be here and call it directly?

I inlined the method into FacetComponent.countFacets. I didn't move the casting logic though. The mergeContributionFromShard method could in theory accept an object and cast it to the right type but a method accepting just a java.lang.Object doesn't feel right to me.

bq. FacetComponent -- now that these skethy "num()" functions are no longer private, can we please get some javadocs on them.

Done.

{quote}
* PivotFacetProcessor
** unless i'm missunderstanding the usage, the way addPivotQueriesAndRanges (and removeUnwantedQueriesAndRanges) works means that every facet.query and facet.range param value (with all localparams) is going to be reparsed over and over and over again for every unique value in every pivot field – just to check the "tag" values and see if it's one that should be computed for this pivot.
This seems really unneccessary – why not parse each param once into a simple datastructure (isn't that what the new ParsedParams" class is designed for?), and then put them in a map by tag available fro mthe request context – just like we did for the stats with StatsInfo.getStatsFieldsByTag(String) ?
** in particular won't this slow down existing requests containing both facet.pivot and facet.range || facet.query) ... even if the later aren't tagged or hung off of the pivots at all? because they'll still get parsed over and over again won't they?
{quote}

You're right. This was horrible and I should've noticed it myself. We now cache the ParsedParam by tags and use them instead of removing unwanted ranges/queries and re-parsing the request.

bq. this logic also seems to completely break instances of facet.query used w/o linking it to a face.tpivot

This is also fixed.

bq. also broken: neither of these requests should result in the facet.query hanging off of the pivots, but because of how StringUtils.contains() is used they both do erroniously...

Also fixed.

{quote}
* tests...
** a lot of code in the new test classes seems to have been copied verbatim from other existing tests – in some cases this is fine, because the copied test logic has been modified to include new params+asserts of the new functionality – but theres still a lot of redundent copy/past cruft w/o any logic changes
*** eg: DistributedFacetPivotQuerySmallTest lines 428-532 seem to be verbatim copied from DistributedFacetPivotSmallTest w/o any additions to test the racet.query logic, or even new negative-assertions that stra facet.queries are hung off by mistake (ie: to catch the bugs i mentioned above)
*** dito for DistributedFacetPivotRangeSmallTest
** there doesn't seem to be any new tests that show hanging both ranges & queries onto a pivot at the same time – let alone combining it with the existing stats logic
** likewise i don't see any testing of using the same tag with multiple facet.query instances (or multiple facet.range instances) and confirming that both get hung off of the same pivot.
{quote}

# I refactored and folded in the test logic back inside the DistributedFacetPivotSmallTest.
# There was really no need to duplicate the PivotFieldComparator, UnorderedEqualityArrayList and ComparablePivotField classes.
# DistributedFacetPivotSmallTest.testNegativeFacetQuery and testNegativeFacetRange test for negative assertions to ensure that extra facet.queries and facet.ranges are not hung by mistake
# DistributedFacetPivotSmallTest.testPivotFacetRangeAndQuery tests range and queries hanging off pivots together along with stats


> Let facet queries hang off of pivots
> ------------------------------------
>
>                 Key: SOLR-4212
>                 URL: https://issues.apache.org/jira/browse/SOLR-4212
>             Project: Solr
>          Issue Type: Sub-task
>          Components: search
>    Affects Versions: 4.0
>            Reporter: Steve Molloy
>            Assignee: Shalin Shekhar Mangar
>             Fix For: 4.9, Trunk
>
>         Attachments: SOLR-4212-multiple-q.patch, SOLR-4212-multiple-q.patch, SOLR-4212.patch, SOLR-4212.patch, SOLR-4212.patch, SOLR-4212.patch, SOLR-4212.patch, SOLR-4212.patch, SOLR-6353-6686-4212.patch, SOLR-6353-6686-4212.patch, SOLR-6353-6686-4212.patch, patch-4212.txt
>
>
> Facet pivot provide hierarchical support for computing data used to populate a treemap or similar visualization. TreeMaps usually offer users extra information by applying an overlay color on top of the existing square sizes based on hierarchical counts. This second count is based on user choices, representing, usually with gradient, the proportion of the square that fits the user's choices.
> The proposition is to use local parameters to specify facet query to apply for pivot which matches a tag set on facet query. Parameter format would look like:
> facet.pivot={!query=r1}category,manufacturer
> facet.query={!tag=r1}somequery
> facet.query={!tag=r1}somedate:[NOW-1YEAR TO NOW]



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org