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: [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]
