iverase commented on PR #1017: URL: https://github.com/apache/lucene/pull/1017#issuecomment-1187570816
Hey @nknize! I went through the PR and my feeling is that it is still half-cooked and I worry about adding a new encoding into core that it is not properly worked on. If later we need to change something we will have a hard time doing it once it is released. My suggestion is to either add it to the sandbox where there are no back-compact requirements or wait for another release so you have more time to work on it. More focus feedback: - Early versions of XYShape where using encoded rectangles to perform bounding box queries. That proved to be wrong (https://github.com/apache/lucene-solr/pull/865) because the encoding is not linear, therefore the spatial relationships between geometries are not preserved in the encoded space. This PR seems to go back to that approach for XYShape doc values which is incorrect. - Lucene does not support any geo faceting, therefore for lucene users the real value for the doc values is the ability to perform IndexOrDocValues queries to speed their queries workload, still the current PR seems to be focusing on supporting faceting, e.g by adding centroid values. IMHO If we want to add those facets we should implement geo faceting in lucene alongside or we are not providing value to lucene users and testing would be very hard. - While the Codec API makes it easy to make changes across minors, we don't have the same flexibility for things that are done on top of the codec API like this binary encoding. Maybe we should take some more time to review the encoding instead of rushing it into 9.3. For instance, we used the index creation major version of the index to change the way facets store the taxonomy array, but this only allows changes on new major versions. -- 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
