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

Support malformed numbers in synthetic _source #90428

Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 27, 2022

This adds support for ignore_malformed to numeric fields other than scaled_float in synthetic _source. Their values are saved to a stored field and loaded to render the _source.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Hi @nik9000, I've created a changelog YAML for you.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 27, 2022
@nik9000 nik9000 mentioned this pull request Sep 27, 2022
50 tasks
@nik9000 nik9000 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Sep 27, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Sep 27, 2022
@elasticsearchmachine
Copy link
Collaborator

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

This adds support for `ignore_malformed` to numeric fields other than
`scaled_float` in synthetic `_source`. Their values are saved to a
stored field and loaded to render the `_source`.
@@ -379,6 +383,7 @@ protected final class NumberSyntheticSourceSupport implements SyntheticSourceSup
private final Function<Number, Number> round;
private final Long nullValue = usually() ? null : randomNumber().longValue();
private final boolean coerce = rarely();
private final boolean ignoreMalformed = rarely();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a specific test for this somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's run rarely like null value support or coerce or whatever. I could could run it on each run in a new test if you'd like. That way we're more likely to catch regressions faster....

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think I'd be happier with explicit tests for each configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened #90565 so we'll have known examples to test in addition to some tests each iteration.

Copy link
Member Author

@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.

@romseygeek could you have another look at this one now that I've merged in my ignore_malformed testing?

@@ -337,75 +337,76 @@ protected Object generateRandomInputValue(MappedFieldType ft) {
}

@Override
protected SyntheticSourceSupport syntheticSourceSupport() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Pulled into a class so it's easier to read. sorry about the big diff.

private final InetAddress nullValue = usually() ? null : randomIp(randomBoolean());
private final boolean ignoreMalformed = rarely();
protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) {
return new IpSyntheticSourceSupport(ignoreMalformed);
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved this to a named class so it'd be a bit easier to read.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for extending the test coverage.

return new SyntheticSourceExample(in, out, this::mapping);
}
protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) {
assumeFalse("scaled_float doesn't support ignore_above with synthetic _source", ignoreMalformed);
Copy link
Contributor

Choose a reason for hiding this comment

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

ignore_malformed not ignore_above, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah!

@nik9000 nik9000 merged commit bc49392 into elastic:main Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Meta label for search team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants