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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
