Skip to content

Conversation

@romseygeek
Copy link
Contributor

As a result of this, we can remove a chunk of code from TypeParsers as well. Tests
for search/index mode analyzers have moved into their own file. This commit also
rationalises the serialization checks for parameters into a single SerializerCheck
interface that takes the values includeDefaults, isConfigured and the value
itself.

Relates to #62988

@romseygeek romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.10.0 labels Oct 5, 2020
@romseygeek romseygeek requested a review from nik9000 October 5, 2020 17:15
@romseygeek romseygeek self-assigned this Oct 5, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Mapping)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Oct 5, 2020
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Everything seems in order. I've not looked as closely as I maybe should - I'm trusting @romseygeek and tests to cover sneaky stuff. The less obvious changes that I saw all make good sense to me.

}
this.positionIncrementGap = positionIncrementGap;
return this;
final TextParams.Analyzers analyzers;
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this below all the Parameter so I don't lose it?

//Check "=" sign wasn't in the pair string
if(kv[0].length() == pair.length()) {
//untyped value
value = URLDecoder.decode(kv[0], StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

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

👍

.addDeprecatedName("max_input_len")
.setValidator(Builder::validateInputLength)
.alwaysSerialize();
.setSerializerCheck((id, ic, v) -> true);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if alwaysSerialize should a shortcut to this new thing - it is easier for me to read the intent of alwaysSerialize than this lambda.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added alwaysSerialize and neverSerialize as shortcuts.

return isSet ? value : defaultValue.get();
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

Are you thinking of removing the old getValue in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the plan, yes

return new PrefixConfig(minChars, maxChars);
}

private static final class FielddataFrequencyFilter implements ToXContent {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek romseygeek force-pushed the mapper/textfield-params branch from c443d8b to 33dde51 Compare October 6, 2020 14:51
@romseygeek
Copy link
Contributor Author

BWC issues with serialization mean that I've had to override toXContentBody and expose a bit more of the internal workings of Parameter than I'd like; I think if we remove the Mapper consistency assertions (see #59427) we can fix this up again in master, but for now we have this nasty sticking plaster.

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek romseygeek force-pushed the mapper/textfield-params branch from 7aa6e71 to 7734125 Compare October 7, 2020 08:28
@romseygeek romseygeek merged commit f4c85e4 into elastic:master Oct 7, 2020
@romseygeek romseygeek deleted the mapper/textfield-params branch October 7, 2020 09:29
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Oct 7, 2020
As a result of this, we can remove a chunk of code from TypeParsers as well. Tests
for search/index mode analyzers have moved into their own file. This commit also
rationalises the serialization checks for parameters into a single SerializerCheck
interface that takes the values includeDefaults, isConfigured and the value
itself.

Relates to elastic#62988
romseygeek added a commit that referenced this pull request Oct 7, 2020
As a result of this, we can remove a chunk of code from TypeParsers as well. Tests
for search/index mode analyzers have moved into their own file. This commit also
rationalises the serialization checks for parameters into a single SerializerCheck
interface that takes the values includeDefaults, isConfigured and the value
itself.

Relates to #62988
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.10.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants