mikemccand commented on a change in pull request #1733:
URL: https://github.com/apache/lucene-solr/pull/1733#discussion_r469977297
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -322,9 +324,14 @@ public FacetLabel getPath(int ordinal) throws IOException {
return res;
}
}
Review comment:
Pre-existing: I don't like that we `return null` up above if the
requested `ordinal` is out-of-bounds. That's dangerous leniency and likely
means the user is refreshing their main `IndexReader` and the `TaxonomyReader`
in the wrong order. It would be better to throw an exception here?
@gautamworah96 could you open a follow-on issue to fix that? Thanks.
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyWriter.java
##########
@@ -494,6 +496,7 @@ private int addCategoryDocument(FacetLabel categoryPath,
int parent) throws IOEx
fullPathField.setStringValue(FacetsConfig.pathToString(categoryPath.components,
categoryPath.length));
d.add(fullPathField);
+ d.add(new BinaryDocValuesField(Consts.FULL, new
BytesRef(FacetsConfig.pathToString(categoryPath.components,
categoryPath.length))));
Review comment:
Could you factor out the `FacetsConfig.pathToString(...)` part in a new
local variable and re-use that? We use it in (at least?) two places here.
----------------------------------------------------------------
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
