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

Projects
None yet
3 participants
@areek
Copy link
Contributor

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

@rjernst

View changes

docs/reference/search/suggesters/completion-suggest.asciidoc Outdated
@@ -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,

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 22, 2014

Member

Maybe use "containing" instead of "representing"?

This comment has been minimized.

Copy link
@areek

areek Oct 22, 2014

Author Contributor

changed

@rjernst

View changes

src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java Outdated
if (NumberType.LONG != numberType && NumberType.INT != numberType) {
throw new ElasticsearchIllegalArgumentException("Weight must be an integer, but was [" + parser.numberValue() + "]");
}
weight = parser.longValue(); // always parse a long to make sure we don't get the overflow value

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 22, 2014

Member

I think you mean "get overflow" not "get the overflow value"?

This comment has been minimized.

Copy link
@areek

areek Oct 22, 2014

Author Contributor

changed to "get overflow"

@rjernst

View changes

src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java Outdated
@@ -288,20 +288,29 @@ public void parse(ParseContext context) throws IOException {
} else {
throw new MapperException("payload doesn't support type " + token);
}
} else if (token == XContentParser.Token.VALUE_STRING) {
if (Fields.CONTENT_FIELD_NAME_OUTPUT.equals(currentFieldName)) {
} else if (token == XContentParser.Token.VALUE_STRING || token == XContentParser.Token.VALUE_NUMBER) {

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 22, 2014

Member

I think it might be cleaner to separate this to one elseif handling the string, and have an additional elsif after handling a direct value? This would eliminate the need for the if/else below for value/string, and would clearly separate what happens in each case (and then you don't need the string checking in the other conditionals as well).

This comment has been minimized.

Copy link
@areek

areek Oct 22, 2014

Author Contributor

separated out handling String and number

@rjernst

View changes

src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java Outdated
}
weight = parser.longValue(); // always parse a long to make sure we don't get the overflow value
if (weight < 0 || weight > Integer.MAX_VALUE) {

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 22, 2014

Member

This seems to be the only "shared" portion for string/value. Perhaps it could be moved out? Or just have a helper method, or even just leave the duplication (it is only 2 lines really, not bad).

This comment has been minimized.

Copy link
@areek

areek Oct 22, 2014

Author Contributor

refactored to a helper method

@rjernst

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2014

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

@rjernst

View changes

src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java Outdated
} 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

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 22, 2014

Member

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

@rjernst

This comment has been minimized.

Copy link
Member

commented Oct 22, 2014

LGTM, just one more minor comment.

@rjernst rjernst removed the review label Oct 22, 2014

@areek

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

@areek you want to get this merged in?

Completion Suggester: Fix CompletionFieldMapper to correctly parse we…
…ight

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

closes #8090

@areek areek force-pushed the areek:fix/8090 branch to 96f1606 Oct 28, 2014

@areek areek merged commit 96f1606 into elastic:master Oct 28, 2014

@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
You can’t perform that action at this time.