mikemccand commented on a change in pull request #1893:
URL: https://github.com/apache/lucene-solr/pull/1893#discussion_r494398560
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/FacetTestCase.java
##########
@@ -56,6 +60,28 @@ public Facets getTaxonomyFacetCounts(TaxonomyReader
taxoReader, FacetsConfig con
return facets;
}
+ public List<List<FacetLabel>> getTaxonomyFacetLabels(TaxonomyReader
taxoReader, FacetsConfig config, FacetsCollector fc) throws IOException {
Review comment:
Thank you for adding this utility method so tests can easily use the new
utility class!
Can we rename this to `getAllTaxonomyFacetLabels`, and add javadoc
explaining that the outer list is one entry per matched hit, and the inner list
is one entry per `FacetLabel` belonging to that hit?
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetCounts.java
##########
@@ -726,6 +743,39 @@ public void testRandom() throws Exception {
IOUtils.close(tw, searcher.getIndexReader(), tr, indexDir, taxoDir);
}
+ private static List<List<FacetLabel>>
sortedFacetLabels(List<List<FacetLabel>> allfacetLabels) {
+ for (List<FacetLabel> facetLabels : allfacetLabels) {
+ Collections.sort(facetLabels);
+ }
+
+ Collections.sort(allfacetLabels, (o1, o2) -> {
+ if (o1 == null) {
Review comment:
Hmm why are these `null` checks necessary? Are we really seeing `null`
in the argument? Oh, I guess this legitimately happens when the hit had no
facets? Maybe add a comment? Hmm, actually, looking at how actual and
expected are populated, neither of them seems to add `null`? One of them
filters out empty list but the other does not?
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetCounts.java
##########
@@ -711,6 +723,11 @@ public void testRandom() throws Exception {
}
}
+ // Test facet labels for each matching test doc
+ List<List<FacetLabel>> actualLabels = getTaxonomyFacetLabels(tr, config,
fc);
+ assertEquals(expectedLabels.size(), actualLabels.size());
Review comment:
Hmm I think `expectedLabels` filters out empty `List<FacetLabel>` but
`actualLabels` does not, so this might false trip?
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetCounts.java
##########
@@ -726,6 +743,39 @@ public void testRandom() throws Exception {
IOUtils.close(tw, searcher.getIndexReader(), tr, indexDir, taxoDir);
}
+ private static List<List<FacetLabel>>
sortedFacetLabels(List<List<FacetLabel>> allfacetLabels) {
+ for (List<FacetLabel> facetLabels : allfacetLabels) {
+ Collections.sort(facetLabels);
+ }
+
+ Collections.sort(allfacetLabels, (o1, o2) -> {
Review comment:
I'm confused why we are sorting the top list? Isn't the top list in
order of the hits? And we want to confirm, for a given `docId` hit, that
expected and actual labels match?
OK, I think I understand: this test does not index anything allowing you to
track which original doc mapped to which `FacetLabel`, so then you cannot know,
per segment, which docs ended up where :)
Given that, I think it's OK to do the top-level sort of all
`List<FacetLabel>` across all hits.
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
