[
https://issues.apache.org/jira/browse/LUCENE-5316?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13812294#comment-13812294
]
Shai Erera commented on LUCENE-5316:
------------------------------------
Few comments:
* In IntRollupFacetsAggregator.rollupValues, I think we shouldn't recurse if
childrenIterator == null, rather than recurse, check for null and return?
** Same for SumScoreFacetsAggregator.
* TopKFacetResultsHandler - the code checks that {{kids != null}}, but as far
as I can tell from the patch, it later does {{childStack[localDepth] = kids}},
and then later sort of assumes {{childStack[localDepth]}} isn't null and calls
.next(). Maybe I'm missing the bigger picture (I haven't applied the patch,
just reviewing it) and the method exits if there are no children?
** The same code is executed afterwards with the comment 'push next kid to the
stack'.
* TopKInEachNodeHandler:
** There's a nocommit which I think can be removed as the surrounding lines are
commented out
** Here too I see that ChildrenIterator is assigned to the childStack, and
later on it's used, assuming it's not null. Maybe it's safe because you assign
ordinalStack[localDepth] = INVALID_ORDINAL, and so the loop doesn't actually
gets to use childStack[localDepth]?
* TestDirectoryTaxonomyReader - why did you have to change the declarations
from TR to DTR?
Some general comments:
* If we're making changes to the API already, how about if we consolidate
getPTA() and getChildren() into a single getTaxonomyTree() (return
TaxonomyTree) with, for now, a single getChildren(int ord) API. It can be
useful for browsing the taxonomy in general, and the search code can use it too.
** I don't see where we make use of the parents() API besides tests and
DirTaxoWriter.getParent(), but this needs to change too -- TaxoWriter does not
need to hold a TaxoTree object, only to be able to tell the parent of an
ordinal (used when indexing the full path ordinals in the category list.
* I think that we should experiment here with a TaxoTree object which holds the
children in a map. Could be that even though the ChildrenIterator may add some
overhead, overall we'll see some gains.
** Also, the title of the issue is about improving the taxonomy tree traversal,
so just moving to getChildren() is not exactly an improvement :).
** We can, if it's too complicated to resolve here, think about how to allow
DirTaxoReader extensions to provide their own TaxoTree object in a separate
issue.
> Taxonomy tree traversing improvement
> ------------------------------------
>
> Key: LUCENE-5316
> URL: https://issues.apache.org/jira/browse/LUCENE-5316
> Project: Lucene - Core
> Issue Type: Improvement
> Components: modules/facet
> Reporter: Gilad Barkai
> Priority: Minor
> Attachments: LUCENE-5316.patch, LUCENE-5316.patch
>
>
> The taxonomy traversing is done today utilizing the
> {{ParallelTaxonomyArrays}}. In particular, two taxonomy-size {{int}} arrays
> which hold for each ordinal it's (array #1) youngest child and (array #2)
> older sibling.
> This is a compact way of holding the tree information in memory, but it's not
> perfect:
> * Large (8 bytes per ordinal in memory)
> * Exposes internal implementation
> * Utilizing these arrays for tree traversing is not straight forward
> * Lose reference locality while traversing (the array is accessed in
> increasing only entries, but they may be distant from one another)
> * In NRT, a reopen is always (not worst case) done at O(Taxonomy-size)
> This issue is about making the traversing more easy, the code more readable,
> and open it for future improvements (i.e memory footprint and NRT cost) -
> without changing any of the internals.
> A later issue(s?) could be opened to address the gaps once this one is done.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
