Skip to content
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

Fix CompletionFieldMapper to correctly parse weight #8197

Merged
merged 1 commit into from Oct 28, 2014

Conversation

areek
Copy link
Contributor

@areek areek commented Oct 22, 2014

Allows weight to be defined as a string representation of a positive integer

closes #8090

@areek areek added the review label Oct 22, 2014
@areek areek self-assigned this Oct 22, 2014
@@ -119,8 +119,9 @@ The following parameters are supported:
might not yield any results, if `input` and `output` differ strongly).

`weight`::
A positive integer, which defines a weight and allows you to
rank your suggestions. This field is optional.
A positive integer or a string representing a positive integer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use "containing" instead of "representing"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@rjernst
Copy link
Member

rjernst commented Oct 22, 2014

I left some comments. The main thing is I would simplify by separating the string/number case, since they really are different other than range checking of the value.

@areek
Copy link
Contributor Author

areek commented Oct 22, 2014

@rjernst Thanks for the feedback, I have updated the PR accordingly.

} catch (NumberFormatException e) {
throw new ElasticsearchIllegalArgumentException("Weight must be a string representing a numeric value, but was [" + parser.text() + "]");
}
weight = weightValue.longValue(); // always parse a long to make sure we don't get the overflow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just "overflow" not "the overflow"?

@rjernst
Copy link
Member

rjernst commented Oct 22, 2014

LGTM, just one more minor comment.

@rjernst rjernst removed the review label Oct 22, 2014
@areek
Copy link
Contributor Author

areek commented Oct 23, 2014

@rjernst Thanks!
I think this is ready, I will merge the fix to all the appropriate branches after a couple of hours, if there is no more objections.

@clintongormley
Copy link

@areek you want to get this merged in?

…ight

 - Allows weight to be defined as a string representation of a positive integer

closes elastic#8090
@areek areek merged commit 96f1606 into elastic:master Oct 28, 2014
@clintongormley clintongormley added the :Search/Suggesters "Did you mean" and suggestions as you type label Mar 19, 2015
@clintongormley clintongormley changed the title Completion Suggester: Fix CompletionFieldMapper to correctly parse weight Fix CompletionFieldMapper to correctly parse weight Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Completion Suggester: Fix CompletionFieldMapper to correctly parse weight
3 participants