-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Refactor of GeohashCellQuery #13393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor of GeohashCellQuery #13393
Conversation
@@ -42,6 +43,8 @@ | |||
|
|||
/** Default for boost to apply to resulting Lucene query. Defaults to 1.0*/ | |||
public static final float DEFAULT_BOOST = 1.0f; | |||
public static final ParseField NAME_FIELD = new ParseField("_name"); | |||
public static final ParseField BOOST_FIELD = new ParseField("boost"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to decide whether ParseFields should be in query builders or query parsers. Good to share these two though given that they are always the same. They should indeed be in some base class.
left a few minor comments, looks good |
@javanna I pushed a commit which addresses your comments |
private String geohash; | ||
private int levels = -1; | ||
private Integer levels = null; | ||
private boolean neighbors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we have sensible defaults for levels and neighbors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a default of false
for neighbors but null
is actually the sensible default for levels
as it means 'do not truncate the given geohash'. The levels
parameter is only there to truncate the precision of the given geohash if required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, thanks
left one tiny comment around default values in the builder, LGTM besides that |
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to #10217 PR goes against the query-refactoring branch
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
Relates to #10217
PR goes against the query-refactoring branch