[GitHub] [lucene] gsmiller commented on a change in pull request #149: LUCENE-9971: SortedSetDocValuesFacetCounts throws exception in case of unseen dimension (unlike other Facet implementations)

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

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



##########
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:
       This gets a little hard to think about, but I there might still be a case where we want to throw here. I'd be curious what you think. If the user has told us _anything_ about the dimension by registering something about it in `FacetsConfig` (i.e., calling some `setXXX` method), then we could check if the expected index field matches. There are two "error" cases here:
   
   1. The user specified a custom index field for a dimension in `FacetsConfig` but this `Facets` instance is using a different field (default or otherwise).
   2. The user didn't specify any custom index field but told us "something else" about the dimension (e.g., that it's hierarchical) and this `Facets` instance is using a non-default index field.
   
   In both of these cases, we know that the dimension is "valid" in the sense that the user set some configuration for it, and we know that we're looking in the wrong index field for it. In this case, I think there's an argument to throw an IAE because it tells the user, "hey, we actually know about this dimension but you're looking in the wrong place!"
   
   For cases where `FacetsConfig` "knows nothing" about the dimension, we have no idea if it's "valid" or not. It could be that the user just wants the default config for it, but it could also be that it's "invalid". So, in all these cases, I think we avoid throwing (like you're doing).
   
   So I think the question is, in cases where we know about the dimension and are certain that the `Facets` instance is looking in the wrong index field for it, should we silently return `null` here or should we throw so that the user knows? I lean towards throwing. Otherwise, the user would think the dimension wasn't found at all even though it could have lots of counts in the field it's actually indexed in. What do you think though?

##########
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:
       We generally use the form `if(isDimIndexed(dim) == false)` instead. There's older code that does `(!condition)` still but it's preferred to explicitly check `== false` (even though your IDE will probably complain about it by default...).
   
   You've got a few checks of this form, but I'll just note it in this one spot :)

##########
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:
       You should be able to leave off `Assert.` in these checks (and remove the import as well). These test cases actually sub-class `Assert` itself, so the import and `Assert.` bit isn't necessary




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