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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
