Introduce DocValuesStatsCollectorManager#15653
Introduce DocValuesStatsCollectorManager#15653gaobinlong wants to merge 2 commits intoapache:mainfrom
Conversation
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left some comments and questions, thanks for working on this!
| @SuppressWarnings("unchecked") | ||
| public S reduce(Collection<DocValuesStatsCollector> collectors) throws IOException { | ||
| if (collectors.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
This is a potential bug that could cause problems down the line? Could we instead return empty stats? Is it even necessary to have this special case? You could instead just remove this conditional?
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| int compareMin(Object a, Object b) { | ||
| if (a instanceof Number numA && b instanceof Number numB) { | ||
| return Double.compare(numA.doubleValue(), numB.doubleValue()); |
There was a problem hiding this comment.
I believe this could lead to precision loss? is the special number case necessary?
| if (a instanceof Number numA && b instanceof Number numB) { | ||
| return Double.compare(numA.doubleValue(), numB.doubleValue()); | ||
| } | ||
| return ((Comparable) a).compareTo(b); |
There was a problem hiding this comment.
This is the same as compareMin ?
| @@ -55,16 +56,21 @@ public class TestDocValuesStatsCollector extends LuceneTestCase { | |||
|
|
|||
There was a problem hiding this comment.
Should we also update testDocsWithSortedValues and testDocsWithSortedSetValues?
|
|
||
| /** | ||
| * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}. | ||
| * |
There was a problem hiding this comment.
Could we expand javadocs to include a usage example for users?
| @Override | ||
| void merge(DocValuesStats<?> other) { | ||
| super.merge(other); | ||
| if (other instanceof SortedLongDocValuesStats o) { |
There was a problem hiding this comment.
is this type of conditional necessary? Does it ever happen that we get a different type than SortedLongDocValuesStats here? This applies to all merge methods.
2 participants
Description
This pr introduces DocValuesStatsCollectorManager and switches TestDocValuesStatsCollector.java to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to #12892.