nknize commented on PR #11753: URL: https://github.com/apache/lucene/pull/11753#issuecomment-1295043124
> I strongly believe that LatLonShape /XYShape should only contain factory methods that work on top of the BKD tree index I see what you're saying, and I really like that idea of separating DocValues and BKD factory methods to their own factory classes. Except currently [LatLonShapeDocValuesQuery](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/LatLonShapeDocValuesQuery.java#L30) and [XYShapeDocValuesQuery](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/XYShapeDocValuesQuery.java#L29) are not public and so their factory methods [already live inside LatLonShape](https://github.com/apache/lucene/blob/5c7edd7f387032485a9fd49508b6c028bff688f0/lucene/core/src/java/org/apache/lucene/document/LatLonShape.java#L262) and [XYShape](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/XYShape.java#L213) factory class, respectively, as does the [ `createDocValuesField` factory methods](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/LatLonShape.ja va#L89). The [javadocs](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/LatLonShape.java#L48) have all of these factory methods listed as their respective `createIndexableFIelds` vs `createDocValuesField`. For a similar reason I had thought about putting the doc value factory methods inside `LatLonShapeDocValues` and `XYShapeDocValues` but these classes are now the concrete docvalue implementations of `ShapeDocValues` so refactoring those DocValues factory methods conflates the purpose of the class. That's why everything was put in `LatLonShape` `XYShape` since there is no purpose for those classes other than factory methods. I suppose an alternative could be to create `LatLonShapeDocValue` and `XYShapeDocValue` (without the 's') factory classes that contain the docvalue factory methods only? I just think having a difference of an 's' could lead to more confusion of the public API. Open to other ideas. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org 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
