[
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 review Hoss. This patch should take care of all the concerns you
outlined.
{quote}
If you don't think it makes sense to keep this method where it is that's fine,
but it is a public method designed for people writting custom request handlers
to use to add faceting to their stuff – so at the very least you should leave a
deprecated stub in it's place that calls your new method.
It also confuses the hell out of me that in the process of moving this method
you made the method signature more complicated to use (more args to understand,
more exceptions to deal with) and got rid of the javadocs so the new more
complicated call signature is even harder to understand – and I, frankly, can't
make sense of why any of that is neccessary (even if should now live in a diff
class because of the inheritence)...
why does the caller have to pass in an empty NamedList to be mutated? why can't
the method just return a NamedList like the old method did?
why does the caller have to instantiate the date & range processors themselves?
in both of the places this method is called, the processors are constructed
just to pass to this method and then thrown away.
So why can't this static method construct them internally (using the request,
docset, params, and response builder from the SimpleFacets object that's also
passed as an arg) and keep the method signature simple?
why does this new method catch & wrap SyntaxErrors but not IOExceptions like
the old method?
(If there reasons for these changes then great – but they aren't clear just
from reading the patch, and there's no javadocs to get guidance from)
{quote}
# I have restored the old method but marked it as deprecated.
# It internally calls the new static helper method to add all kinds of facets.
# The signature is also simplified. It now accepts a SimpleFacets class and
constructs all other required processors internally and returns a NamedList
like the old method did.
# Not wrapping IOException was a mistake. This is fixed.
{quote}
I don't see a class called "RangeFacet" ... it looks like you talking about
RangeFacetValue?
FWIW: PivotFacetValue doesn't extend FacetBase because it models a single
(value,count) pair in the context of a PivotFacetField (which may be
hierarchical) ... you may be thinking of "PivotFacet"
... in which case, should this class atually be named "RangeFacet" ?
either way: can we please beef up the javadocs of this class to be crystal
clear about what it's modeling – at first glance I thought it was just modeling
a facet.range param (with the field, start, end, calculator) before I realized
it not only models the field + the indiviual buckets, but also the values in
those buckets (but not the other params)
Isn't RangeFacetValue.fieldName redundent here? - it's already handled by
FacetBase.facetOn.
I don't think there is any reason to initialize shardFieldValues in
mergeContributionFromShard before the rangeFacet = rangeFromShard short-circut,
it's just wasted cycles the first time the method gets called, correct?
{quote}
I further refactored the code into into three separate classes to divide the
responsibilities clearly.
# The first called RangeFacetRequest models a single facet.range request along
with all request parameters, appropriate calculator and ranges (gaps).
Instances of this class are now created once in FacetComponent.prepare and
cached in the context and re-used for non-distrib facet.range calculation as
well as in pivot facet classes.
# The second RangeFacetRequest.DistribRangeFacet models the full 'facet_ranges'
to be returned to the user for a distrib request and has helper methods for
merging contributions of individual shards.
# The third RangeFacetRequest.FacetRange models a single range i.e. an
individual gap for which the count has to be computed. A list of this class is
created by the calculator (inside RangeFacetRequest's constructor)
This is actually almost the same as the framework in the Analytics component
with some minor modifications. Once this issue is committed, I'll open issues
to refactor common code in a single place and re-use. It's funny that I started
writing a class to pre-compute the gaps and ended up with almost the same code
as RangeFacetRequest in Analytics but a little uglier so I threw that code away
and started from the Analytics Component's design and tweaked it to fit our
use-case.
{quote}
why a "Map<String, RangeFacetValue>" for rangeCounts instead of a NamedList?
it seems like a really bad idea for these to not be returned in a deterministic
order – it should be in the same order as the top level facet.range results
(which is hte order of the facet.range params in the request)
no idea if this HashMap usage currently causes problems in the shard merging,
but it smells fishy – and either way the user should be able to count on
getting the range facets back in the same order the asked for them
{quote}
This is now a LinkedHashMap and its key/values are added in the same order as
the NamedList from a shard's response. Though I do not agree about the user
counting on the same order because this is not a list but a complex object in
all our responses. But I digress.
{quote}
i didn't dig into the full ramifications of this, but you've got
PivotFacetValue constructing RangeFacetValue objects and passing the response
key in as the "fieldName" argument to the constructor – those aren't the same
thing. Either the method call here is wrong, or the constructor arg should be
renamed – either way any usage of this "fieldName" constructor arg and internal
usage should be sanity checked to ensure it's used consistnetly everywhere
{quote}
This is no longer an issue since we use the DistribRangeFacet class which has
no need for a key at all.
{quote}
are there tests of using a diff response key from the fieldName when hanging
ranges under pivots? (both single node and cloud?)
{quote}
Yes, see DistributedFacetPivotSmallTest.testFacetPivotRange() and
SolrExampleTests.testPivotFacetsRanges()
{quote}
in PivotFacetValue.mergeContributionFromShard, the dozen or so lines of looping
over each shard's response to either construct a new RangeFacetValue or call
RangeFacetValue.mergeContributionFromShard seems like it could/should be
refactored into a static helper method (in the RangeFacetValue class)
{quote}
That bit of code has been refactored into
DistribRangeFacet.mergeFacetRangesFromShardResponse method.
{quote}
why is PivotFacetValue.createFromNamedList conditionally calling
RangeFacetValue.mergeContributionFromShard?
{quote}
Laziness on my part, really. Now that this bit of code has been refactored into
DistribRangeFacet.mergeFacetRangesFromShardResponse, I use the same here as
well. The extra null check wouldn't hurt here.
{quote}
Since the input to PivotFacetHelper.mergeQueryCounts is already in the correct
response format (ie: no special objects need constructed to model the simple
counts) can't it just return "querycounts" if "receivingMap" is null? (instead
of looping & adding)
{quote}
Good point, done.
{quote}
With the current patch, something is wrong with how localparams are dealt with
when trying to use facet.ranges inside of pivots. These queries are examples of
doing range queries on the same field with diff gap params, and (independently)
doing a pivot
{quote}
Fixed. I am not sure why that happened but once I refactored the code to do all
the parsing and calculation in prepare, I could not reproduce the issue.
{quote}
I haven't seen this piece of code yet, but it sounds like it only prevents the
"tags" from being parsed over and over again – from what I've seen so far,
RangeFacetProcessor.getFacetRangeCounts(ParsedParams,NamedList) which delegates
to
RangeFacetProcessor.getFacetRangeCounts(SchemaField,RangeEndpointCalculator,ParsedParams)
is still going to cause a new RangeEndpointCalculator to be constructed, and
the exact same start/end/include/etc... Strings to be parsed over and over and
over again, for every pivot value, at every level. These things should really
just be parsed once in prepare(), put into the request context, and reused at
every level of every pivot.
{quote}
This is also done as I noted earlier in my comment. The class RangeFacetRequest
is the one to be looked at.
> 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: 5.3, 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,
> 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: [email protected]
For additional commands, e-mail: [email protected]
