[
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: [email protected]
For additional commands, e-mail: [email protected]
