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

Make the `index` property a boolean. #16161

Closed

Conversation

Projects
None yet
3 participants
@jpountz
Copy link
Contributor

commented Jan 21, 2016

With the split of string into text and keyword, the index property can
only have two values and should be a boolean.

Make the `index` property a boolean.
With the split of `string` into `text` and `keyword`, the `index` property can
only have two values and should be a boolean.
@rjernst

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

Looks good. Maybe add a backcompat test for the old ways that were supported, and see it still works with created version before 3.0?

}
/* Only protected so that string can override it */
protected Object indexTokenizeOption(boolean indexed, boolean tokenized) {
return Boolean.valueOf(indexed);

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

Boxing of primitive booleans always leads to the same references as this is guaranteed by the JLS so the explicit call to Boolean#valueOf is not needed (however the OpenJDK implementation of the boxing conversion here is to just insert a call to Boolean#valueOf but that's just an implementation detail).

If the value p being boxed is true, false, [...], then let r1 and r2 be the results of any two boxing conversions of p. It is always the case that r1 == r2.

node.put("index", false);
break;
default:
throw new IllegalArgumentException("Can't parse [index] value [" + index + "], expected [true], [false], [no], [not_analyzed] or [analyzed]");

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

Should the exception message contain the input index0 instead of the normalized index?

} else {
throw new MapperParsingException("wrong value for index [" + index + "] for field [" + fieldName + "]");
index = Strings.toUnderscoreCase(index);

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

I like how in StringFieldMapper you use a different variable for the normalized input, and I think that you should do the same here?

case "no":
return false;
default:
throw new IllegalArgumentException("Can't parse [index] value [" + index + "], expected [true], [false], [no], [not_analyzed] or [analyzed]");

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

I also think that this should log the input instead of the normalized input.

@@ -146,6 +146,27 @@ public StringFieldMapper build(BuilderContext context) {
@Override
public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException {
StringFieldMapper.Builder builder = stringField(name);
// hack for the fact that string can't just accept true/false for
// the index property and still accepts no/not_analyzed/analyzed
final Object indexO = node.remove("index");

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

Maybe inputIndex instead of index0?

// the index property and still accepts no/not_analyzed/analyzed
final Object indexO = node.remove("index");
if (indexO != null) {
final String index = Strings.toUnderscoreCase(indexO.toString());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

Maybe normalizedIndex instead of index?

@@ -267,6 +267,12 @@ float by default instead of a double. The reasoning is that floats should be
more than enough for most cases but would decrease storage requirements
significantly.

==== `index` property

On all types but `string`, the `index` property now accepts `true`/`false`

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 21, 2016

Member

Maybe insert the word "only"?

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2016

@rjernst @jasontedor Thanks for the reviews, I pushed a new commit.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

LGTM.

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2016

Fixed via 2098608

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.