[GitHub] [lucene] MarcusSorealheis commented on pull request #12162: Add LatLonField class to index both LatLonPoint and LatLonDocValues

["MarcusSorealheis (via GitHub)" <gi...@apache.org>]

MarcusSorealheis commented on PR #12162:
URL: https://github.com/apache/lucene/pull/12162#issuecomment-1437925210

   I've been doing some research and hope MongoDB can help make an impact here soon for all of our customers. However, not speaking for MongoDB and only for myself, I want to weigh in on this conversation for three reasons in the hope that we can eventually figure this one out:
   
   1. @rmuir is one of my heroes(yea, I'm a nerd) and I depended immensely on code he wrote or mentored when I was trying to pull myself up out of poverty with free, open source NoSQLs.  
   
   2. @iverase is such an important and valuable member of the community, and the work he has been doing in the past few years has been good for Lucene's continued success and I appreciate it. 
   
   3. Although I admit to not knowing much, I have my own technical point of view on `newGeometryQuery` that I hope can inform the discussion. Given the fact that I am inclined to be pro (non-binding) the direction, I will start with my opinion of cons, and follow up with pros.
   
   Cons:
   
   - The name is not completely self-evident of the method's function. While it does conform to a pattern used in Lucene over many years to create a static factory method for creating query objects, I wonder if there is a better way than how it's been done. I don't think we should keep calling these things `new` in perpetuity unless they do some really new stuff. It's been years. Such a discussion is probably out of scope for this PR, but I wanted to raise it in case the name I hate is part of the averse reaction from @rmuir. 
   
   - As @rmuir said, it doesn't really do anything. I'm generally an advocating of not adding code that doesn't do anything unless it improves maintainability or modularity. It's one of the reasons I have not proposed much to Lucene.
   
   Pros:
   
   - This factory method requires slightly less code. Although some places look like two lines, that's only because of formatting best practices. The benefits are clearer when stacked as they are in the diff:
   
   | Version | Query|
   | --- | --- |
   | PR | `return newGeometryQuery(field, queryRelation, Arrays.stream(polygons).toArray(Polygon[]::new));` | 
   | Main | `return LatLonPoint.newGeometryQuery(field, queryRelation, Arrays.stream(polygons).toArray(Polygon[]::new));`|
   
   - The method potentially decouples client code from the implementation. That's important because new relations and new geometries could emerge. I don't know what they would possibly be as of writing this comment. I will read up on that. 
   
   - `newGeometryQuery()`'s primary responsibility is to create and return an object (`Query`). However, theoretically, it could be modified in the future to do more generic work to enhance geometric queries provided it was safe. I think that's a reach pro more than anything.
   
   - It's consistent. 
   
   I'm also working on being more of a peacemaker in technical discussions than a tree shaker. A few too many coconuts popped me in the head. 
   
   
   


-- 
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-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org