goankur commented on pull request #1733: URL: https://github.com/apache/lucene-solr/pull/1733#issuecomment-696420384
Thanks @gautamworah96 for this impactful change and @mikemccand for reviewing it. A few thoughts 1. This change disables STORED fields part but keeps the POSTINGS part here `fullPathField = new StringField(Consts.FULL, "", Field.Store.NO); ` which is unnecessary as postings are already enabled for facet labels in [FacetsConfig#L364-L399](https://github.com/apache/lucene-solr/blob/master/lucene/facet/src/java/org/apache/lucene/facet/FacetsConfig.java#L364-L366) including [dimension drill-down](https://github.com/apache/lucene-solr/blob/master/lucene/facet/src/java/org/apache/lucene/facet/FacetsConfig.java#L88). So I propose we get rid of the `fullPathField` altogether. 2. For maintaining backwards compatibility, we can read facet labels from new BinaryDocValues field, falling back to old StoredField if BinaryDocValues field does not exist or has no value for the docId. The performance penalty of doing so should be acceptable. Alternatively we can implement a special merge policy that takes care of moving data from old Stored field to BinaryDocValues field at the time of merge but that might be tricky to implement. ---------------------------------------------------------------- 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
