[GitHub] [lucene] sqshq commented on a change in pull request #149: LUCENE-9971: Inconsistent SSDVFF and Taxonomy facet behavior in case of unseen dimension

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

sqshq commented on a change in pull request #149:
URL: https://github.com/apache/lucene/pull/149#discussion_r641998363



##########
File path: lucene/facet/src/java/org/apache/lucene/facet/taxonomy/TaxonomyFacets.java
##########
@@ -110,17 +109,10 @@ public boolean siblingsLoaded() {
     return children != null;
   }
 
-  /**
-   * Throws {@code IllegalArgumentException} if the dimension is not recognized. Otherwise, returns
-   * the {@link DimConfig} for this dimension.
-   */
-  protected FacetsConfig.DimConfig verifyDim(String dim) {
+  /** Returns false if the dimension was not indexed into {@link #indexFieldName} field. */
+  protected boolean isDimIndexed(String dim) {

Review comment:
       Yes, I like the idea and it seems that would provide a good balance between prior strict assertion and "always return `null`" solution. What do you think about this implementation? https://github.com/apache/lucene/commit/38e54f73f00b518a617ab6f6231639adcd515aa2

##########
File path: lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java
##########
@@ -133,7 +133,10 @@ private int rollup(int ord) throws IOException {
 
   @Override
   public Number getSpecificValue(String dim, String... path) throws IOException {
-    DimConfig dimConfig = verifyDim(dim);
+    if (!isDimIndexed(dim)) {

Review comment:
       Thanks for pointing that out, I fixed that! Btw might be a good idea to add this rule to Lucene's check style settings, so `./gradlew check` can spot this automatically?

##########
File path: lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetAssociations.java
##########
@@ -168,17 +169,8 @@ public void testWrongIndexFieldName() throws Exception {
     IndexSearcher searcher = newSearcher(reader);
     searcher.search(new MatchAllDocsQuery(), fc);
     Facets facets = new TaxonomyFacetSumFloatAssociations(taxoReader, config, fc);
-    expectThrows(
-        IllegalArgumentException.class,
-        () -> {
-          facets.getSpecificValue("float");
-        });
-
-    expectThrows(
-        IllegalArgumentException.class,
-        () -> {
-          facets.getTopChildren(10, "float");
-        });
+    Assert.assertEquals(-1, facets.getSpecificValue("float"));

Review comment:
       :+1: these asserts were removed with the recent changes, but I checked that there is no more `Assert.` anywhere in my code.




-- 
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