[
https://issues.apache.org/jira/browse/LUCENE-5745?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14031789#comment-14031789
]
David Smiley commented on LUCENE-5745:
--------------------------------------
I took a look at this today. Your code was good in that it was how I thought
it would be. When I debugged why it didn't work, I made a couple realizations:
* cell.remove() wasn't implemented on the CellIterator implementation returned
from LegacyPrefixTree when the query shape is a Point (FilteredCellIterator). I
had LPT augment it via an anonymous class.
* findSubCellsToVisit() wasn't quite right; I changed the abstraction so
up-front IsWithin could customize the shape that is used for
getTreeCellIterator(shape).
* It may not have mattered to get the tests to pass but I realized
cell.remove() needed to be invoked in a few other cases as well.
I also made some basic refactorings like inlining addIntersectingChildren().
The lines of code in AVPTF went from 390 to 277 (most of which you already
did), which is pretty darned sweet. This refactoring also means a reduction in
allocated objects -- AVPT introduces no new allocation on a per-cell or
per-term basis.
BUT, I couldn't get the IsWithin predicate to work. I debugged further and
found out why. Essentially, IsWithin wants to have more control over
descending down the SPT, even when the current cell is a leaf. But
TreeCellIterator can't be told to go down, only to not go down. And it won't
go down when the current cell is a leaf. I think I need to monkey with the
APIs a bit to make this work. I'm currently leaning towards TreeCellIterator
being an interface (current code moving to an "Impl" suffix) that gives the
caller control over descending, independent of wether the cell is a leaf.
Then, SPT.getTreeCellIterator would return an actual TreeCellIterator.
> Refactoring AbstractVisitingPrefixTreeFilter code using cellIterator.
> ---------------------------------------------------------------------
>
> Key: LUCENE-5745
> URL: https://issues.apache.org/jira/browse/LUCENE-5745
> Project: Lucene - Core
> Issue Type: Improvement
> Components: modules/spatial
> Reporter: Varun V Shenoy
> Priority: Minor
> Fix For: 5.0
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> The AbstractVisitingPrefixTreeFilter (used by RPT's Intersects, Within,
> Disjoint) really should be refactored to use the new CellIterator API as it
> will reduce the amount of code and should make the code easier to follow
> since it would be based on a well-known design-pattern (an iterator). It
> currently uses a VNode and VNode Iterator.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
