gsmiller commented on a change in pull request #747: URL: https://github.com/apache/lucene/pull/747#discussion_r826440044
########## File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java ########## @@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path) * indexed, for example depending on the type of document. */ public abstract List<FacetResult> getAllDims(int topN) throws IOException; + + /** + * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that + * dimension matched + */ + public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException { Review comment: I like the approach of providing a default implementation here so existing sub-classes will be fully backwards-compatible (and they don't need to worry about providing an implementation if this suits their needs). It might be nice to mention explicitly in the javadoc that sub-classes may _want_ to override this implementation though with a more efficient one if they're able. ########## File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java ########## @@ -104,6 +104,65 @@ public void testBasic() throws Exception { "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", facets.getTopChildren(10, "b").toString()); + // test getAllDims Review comment: Thank you for adding so much testing, including coverage for existing `getAllDims` functionality! ########## File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java ########## @@ -104,6 +104,65 @@ public void testBasic() throws Exception { "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", facets.getTopChildren(10, "b").toString()); + // test getAllDims + List<FacetResult> results = facets.getAllDims(10); + assertEquals(2, results.size()); + assertEquals( + "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", + results.get(0).toString()); + assertEquals( + "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n", + results.get(1).toString()); + + // test getAllDims(1, 0) with topN = 0 + expectThrows( Review comment: Ouch! This seems like a poor (existing) experience for users. Would you mind creating a Jira to track this? We should probably change this behavior to throw an `IllegalArgumentException` at least instead of just an NPE. Thanks for uncovering this! ########## File path: lucene/facet/src/test/org/apache/lucene/facet/range/TestRangeFacetCounts.java ########## @@ -243,6 +243,24 @@ public void testLongGetAllDims() throws Exception { "dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n", result.get(0).toString()); + // test getAllDims(1) + List<FacetResult> test1Child = facets.getAllDims(1); + assertEquals(1, test1Child.size()); + assertEquals( + "dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n", + test1Child.get(0).toString()); + + // test default implementation of getTopDims + List<FacetResult> topNDimsResult = facets.getTopDims(1, 1); + assertEquals(1, topNDimsResult.size()); + assertEquals( Review comment: minor: Since `FacetResult` properly implements `equals`, you could just do `assertEquals(test1Child, topNDimsResult)`. This makes it slightly more obvious to the reader that you expect the exact same behavior as `getAllDims` ########## File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java ########## @@ -104,6 +104,65 @@ public void testBasic() throws Exception { "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", facets.getTopChildren(10, "b").toString()); + // test getAllDims + List<FacetResult> results = facets.getAllDims(10); + assertEquals(2, results.size()); + assertEquals( + "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", + results.get(0).toString()); + assertEquals( + "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n", + results.get(1).toString()); + + // test getAllDims(1, 0) with topN = 0 + expectThrows( + NullPointerException.class, + () -> { + facets.getAllDims(0); + }); + + // test getTopDims(10, 10) and expect same results from getAllDims(10) + List<FacetResult> allDimsResults = facets.getTopDims(10, 10); + assertEquals(2, results.size()); + assertEquals( + "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n", + allDimsResults.get(0).toString()); + assertEquals( + "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n", + allDimsResults.get(1).toString()); + + // test getTopDims(2, 1) + List<FacetResult> topDimsResults = facets.getTopDims(2, 1); Review comment: minor: similar comment about relying on `FacetResult` equality here instead -- 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: issues-unsubscr...@lucene.apache.org 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