gsmiller commented on a change in pull request #191:
URL: https://github.com/apache/lucene/pull/191#discussion_r661692139
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/TestLongValueFacetCounts.java
##########
@@ -556,23 +544,37 @@ public void testRandomMultiValued() throws Exception {
return cmp;
});
if (random().nextBoolean()) {
- topN = valueCount;
+ topN = docCount;
} else {
- topN = random().nextInt(valueCount);
+ topN = random().nextInt(docCount);
}
actual = facetCounts.getTopChildrenSortByCount(topN);
assertSame(
"id " + minId + "-" + maxId + ", sort facets by count",
expectedCounts,
expectedChildCount,
- totCount,
+ expectedTotalCount,
actual,
topN);
}
r.close();
dir.close();
}
+ private void setExpectedFrequencies(long[] values, Map<Long, Integer>
expected) {
+ long previousValue = 0;
+ for (int j = 0; j < values.length; j++) {
+ if (j == 0 || previousValue != values[j]) {
+ Integer curCount = expected.get(values[j]);
Review comment:
Consider using getOrDefault here with a default value of `0` to avoid
the conditional check/assignment below.
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/TestLongValueFacetCounts.java
##########
@@ -556,23 +544,37 @@ public void testRandomMultiValued() throws Exception {
return cmp;
});
if (random().nextBoolean()) {
- topN = valueCount;
+ topN = docCount;
} else {
- topN = random().nextInt(valueCount);
+ topN = random().nextInt(docCount);
}
actual = facetCounts.getTopChildrenSortByCount(topN);
assertSame(
"id " + minId + "-" + maxId + ", sort facets by count",
expectedCounts,
expectedChildCount,
- totCount,
+ expectedTotalCount,
actual,
topN);
}
r.close();
dir.close();
}
+ private void setExpectedFrequencies(long[] values, Map<Long, Integer>
expected) {
Review comment:
Consider a more idiomatic approach here by returning the expected map
instead of populating one passed in as a side-effect. From what I can tell
looking at the calling code, there's no need to populate an existing map. It
would be a bit cleaner and easier to read if you create the expected map here
and return it.
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/TestLongValueFacetCounts.java
##########
@@ -394,7 +394,7 @@ public void testRandomMultiValued() throws Exception {
}
for (int j = 0; j < values[i].length; j++) {
- long value = TestUtil.nextLong(random(), -maxValue, maxValue);
+ Long value = TestUtil.nextLong(random(), -maxValue, maxValue);
Review comment:
Can this also be changed back to a `long`?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
