You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Ashvin A <aa...@gmail.com> on 2015/08/24 19:25:39 UTC
Review Request 37720: Lucene:
GEODE-11-Add-a-basic-score-based-aggregator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37720/
-----------------------------------------------------------
Review request for geode and Dan Smith.
Repository: geode
Description
-------
Aggregator merges results returned from shards and orders the result based on lucene hit score
Diffs
-----
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneQueryResults.java d660a4b
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneResultStruct.java a5b16b7
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneQueryResultsImpl.java ecb6370
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneResultStructImpl.java 0db8f97
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/ResultMerger.java PRE-CREATION
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java PRE-CREATION
gemfire-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/filesystem/TopDocsMergeJUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/37720/diff/
Testing
-------
Thanks,
Ashvin A
Re: Review Request 37720: Lucene:
GEODE-11-Add-a-basic-score-based-aggregator
Posted by Ashvin A <aa...@gmail.com>.
> On Aug. 24, 2015, 8:59 p.m., Dan Smith wrote:
> > gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java, line 29
> > <https://reviews.apache.org/r/37720/diff/1/?file=1048453#file1048453line29>
> >
> > Is is possible to lose a document if two keys have the same hash code? If so, this risks losing a doc. If not, why include this hashcode comparison? Probably better to leave it out and just test adding docs with duplicate scores. I believe priority queue should handle this.
Good point. Based on my understanding, the merger will operate on disjoint keysets received from shards. So I guess I should not worry about duplicate keys while merging and skip this check.
While this is true, the comparator is used for ordering the results and will not loose any doc. I will add a test for duplicate keys for coverage.
Thanks !
- Ashvin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37720/#review96205
-----------------------------------------------------------
On Aug. 24, 2015, 5:25 p.m., Ashvin A wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37720/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 5:25 p.m.)
>
>
> Review request for geode and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Aggregator merges results returned from shards and orders the result based on lucene hit score
>
>
> Diffs
> -----
>
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneQueryResults.java d660a4b
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneResultStruct.java a5b16b7
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneQueryResultsImpl.java ecb6370
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneResultStructImpl.java 0db8f97
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/ResultMerger.java PRE-CREATION
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java PRE-CREATION
> gemfire-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/filesystem/TopDocsMergeJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37720/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashvin A
>
>
Re: Review Request 37720: Lucene:
GEODE-11-Add-a-basic-score-based-aggregator
Posted by Ashvin A <aa...@gmail.com>.
> On Aug. 24, 2015, 8:59 p.m., Dan Smith wrote:
> > gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java, line 29
> > <https://reviews.apache.org/r/37720/diff/1/?file=1048453#file1048453line29>
> >
> > Is is possible to lose a document if two keys have the same hash code? If so, this risks losing a doc. If not, why include this hashcode comparison? Probably better to leave it out and just test adding docs with duplicate scores. I believe priority queue should handle this.
>
> Ashvin A wrote:
> Good point. Based on my understanding, the merger will operate on disjoint keysets received from shards. So I guess I should not worry about duplicate keys while merging and skip this check.
>
> While this is true, the comparator is used for ordering the results and will not loose any doc. I will add a test for duplicate keys for coverage.
>
> Thanks !
I misunderstood your comment. So ignore my earlier reply. A doc will not be lost if two keys with the same hashcode are seen. I have added a test for this.
- Ashvin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37720/#review96205
-----------------------------------------------------------
On Aug. 24, 2015, 5:25 p.m., Ashvin A wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37720/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 5:25 p.m.)
>
>
> Review request for geode and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Aggregator merges results returned from shards and orders the result based on lucene hit score
>
>
> Diffs
> -----
>
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneQueryResults.java d660a4b
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneResultStruct.java a5b16b7
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneQueryResultsImpl.java ecb6370
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneResultStructImpl.java 0db8f97
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/ResultMerger.java PRE-CREATION
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java PRE-CREATION
> gemfire-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/filesystem/TopDocsMergeJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37720/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashvin A
>
>
Re: Review Request 37720: Lucene:
GEODE-11-Add-a-basic-score-based-aggregator
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37720/#review96205
-----------------------------------------------------------
Ship it!
Looks pretty good - I had one comment on the use of hashcode, below.
gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java (line 29)
<https://reviews.apache.org/r/37720/#comment151527>
Is is possible to lose a document if two keys have the same hash code? If so, this risks losing a doc. If not, why include this hashcode comparison? Probably better to leave it out and just test adding docs with duplicate scores. I believe priority queue should handle this.
- Dan Smith
On Aug. 24, 2015, 5:25 p.m., Ashvin A wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37720/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 5:25 p.m.)
>
>
> Review request for geode and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Aggregator merges results returned from shards and orders the result based on lucene hit score
>
>
> Diffs
> -----
>
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneQueryResults.java d660a4b
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneResultStruct.java a5b16b7
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneQueryResultsImpl.java ecb6370
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneResultStructImpl.java 0db8f97
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/ResultMerger.java PRE-CREATION
> gemfire-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/mergers/TopDocsResultMerger.java PRE-CREATION
> gemfire-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/filesystem/TopDocsMergeJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37720/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashvin A
>
>