[ https://issues.apache.org/jira/browse/LUCENE-6930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15119397#comment-15119397 ]
Michael McCandless commented on LUCENE-6930: -------------------------------------------- Thanks [~nknize], this is a nice (and large!) change. The sole "R" in this javadoc left me hanging a bit ;) {noformat} /** * GeoTerms are coded as the following: * * R */ {noformat} Can you update {{GeoPointDistanceQuery}} javadocs explaining the max radius limit? I.e. that the circle projected on the earth's surface cannot wrap around and touch itself again (if I understand that right!)? Can we move {{GeoPointTokenStream}} under {{o.a.l.document}} and make it package private? (And make {{TermEncoding}} public elsewhere.) Can all other ctors of {{GeoPointField}} just forward to the primary ("takes everything") ctor call, i.e. call {{this(...)}} instead of {{super(...)}}? Also, can we break this compound ternary operator into a static helper method?: {noformat} super(name, stored == Store.YES ? termEncoding == GeoPointTokenStream.TermEncoding.PREFIX ? PREFIX_TYPE_STORED : NUMERIC_TYPE_STORED : termEncoding == GeoPointTokenStream.TermEncoding.PREFIX ? PREFIX_TYPE_NOT_STORED : NUMERIC_TYPE_NOT_STORED); {noformat} E.g. maybe {{getFieldType}}. Should it be an error if you pass a custom {{FieldType}} to {{GeoPointField}} that disabled indexing? I.e. catch that up front, where we check DV type and numeric type, and then remove this: {noformat} if (fieldType().indexOptions() == IndexOptions.NONE) { // Not indexed return null; } {noformat} from the {{tokenStream}} method? Can we deprecate the {{GeoPointField}} ctors that take {{TermEncoding}}? (You should use/migrate to the default ctor that uses the better {{PREFIX}} encoding). {{GeoUtils.longToByteArray}} and {{.fromByteArray}} and {{GeoEncodingUtils.geoTermToString}} look dead? This comment confuses me: {noformat} // start shift at 61 private short shift; {noformat} Does it really start at 61? Seems like ({{computeMaxShift}}) it's either 45 (for a large bbox) or 36 (for a not-large bbox)? Can we move the comment down to where we actually do assign to shift? > Decouple GeoPointField from NumericType > --------------------------------------- > > Key: LUCENE-6930 > URL: https://issues.apache.org/jira/browse/LUCENE-6930 > Project: Lucene - Core > Issue Type: Improvement > Reporter: Nicholas Knize > Attachments: LUCENE-6930.patch, LUCENE-6930.patch, LUCENE-6930.patch, > LUCENE-6930.patch, LUCENE-6930.patch > > > {{GeoPointField}} currently relies on {{NumericTokenStream}} to create prefix > terms for a GeoPoint using the precision step defined in {{GeoPointField}}. > At search time {{GeoPointTermsEnum}} recurses to a max precision that is > computed by the Query parameters. This max precision is never the full > precision, so creating and indexing the full precision terms is useless and > wasteful (it was always a side effect of just using indexing logic from the > Numeric type). > Furthermore, since the numerical logic always stored high precision terms > first, the recursion in {{GeoPointTermsEnum}} required transient memory for > storing ranges. By moving the trie logic to its own {{GeoPointTokenStream}} > and reversing the term order (such that lower resolution terms are first), > the GeoPointTermsEnum can naturally traverse, enabling on-demand creation of > PrefixTerms. This will be done in a separate issue. -- This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org