[GitHub] [lucene] gsmiller commented on a change in pull request #131: LUCENE-9953: Make FacetResult#value accurate for LongValueFacetCounts

[GitBox <gi...@apache.org>]

gsmiller commented on a change in pull request #131:
URL: https://github.com/apache/lucene/pull/131#discussion_r630606547



##########
File path: lucene/facet/src/test/org/apache/lucene/facet/TestLongValueFacetCounts.java
##########
@@ -429,15 +429,15 @@ public void testRandomMultiValued() throws Exception {
       int expectedChildCount = 0;
       int expectedTotalCount = 0;
       for (int i = 0; i < valueCount; i++) {
-        if (values[i] != null) {
+        if (values[i] != null && values[i].length > 0) {

Review comment:
       Thanks @gautamworah96 . I'm not sure I fully understand the suggestion to add a duplicate long value to test the change? The current bug is that, whenever a document has multiple values for a particular field (they don't need to be the _same_ value), we'll count that document multiple times (once for each value it contains) in FacetResult#value. So I think it should be sufficient to just make sure documents contain multiple values to trip the bug, which is happening in Line 396 in this test case. The change I made to the test case reflects the proper logic for populating FacetResult#value, and does in fact fail if my change is not present.
   
   Does this make sense? Maybe I'm missing your point. If so, I apologize!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org