Skip to content

Comprehensively test supported/unsupported field type:agg combinations #52493

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

Merged
merged 8 commits into from
Feb 20, 2020

Conversation

polyfractal
Copy link
Contributor

This adds a test to AggregatorTestCase that allows us to programmatically verify that an aggregator supports or does not support all available field types. It fetches the list of registered field type parsers, creates a MappedFieldType from the parser and then attempts to run a basic agg against the field.

A supplied list of supported VSTypes are then compared against the output (success or exception) and suceeds or fails the test accordingly.

Still a WIP, need to tidy up, add javadocs, add another "main" agg or two (histo, etc). Putting it up in case folks want to have a look before I spend more time on it.

Note: to make this work, the PR cherry-picks over the getValuesSourceType() component of #51503. This will be added in eventually from the VS refactor branch, but is technically "unused" code outside of the tests until then.

Note 2: does not support field types added from a module at the moment.

not-napoleon and others added 2 commits February 18, 2020 16:30
This adds a test to AggregatorTestCase that allows us to programmatically
verify that an aggregator supports or does not support a particular
field type.  It fetches the list of registered field type parsers,
creates a MappedFieldType from the parser and then attempts to run
a basic agg against the field.

A supplied list of supported VSTypes are then compared against the
output (success or exception) and suceeds or fails the test accordingly.
@polyfractal polyfractal added >test Issues or PRs that are addressing/adding tests WIP :Analytics/Aggregations Aggregations labels Feb 18, 2020
@elasticmachine
Copy link
Collaborator

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

return Collections.emptyList();
}

public void testSupportedFieldTypes() throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The meat of the PR is here.

@@ -92,14 +94,6 @@ public RangeFieldType fieldType() {
return (RangeFieldType)fieldType;
}

@Override
public Builder docValues(boolean docValues) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we think this is a bug, I'll open a separate PR fixing this

@polyfractal polyfractal marked this pull request as ready for review February 19, 2020 18:28
@polyfractal polyfractal removed the WIP label Feb 19, 2020
@polyfractal
Copy link
Contributor Author

Tidied up, added javadocs, and cleaned up the comments that were left.

For posterity, we also decided to remove the CoreValuesSourceTypes that weren't in use (IP, BOOLEAN, DATE) and return the appropriate type today (BYTES/NUMERIC) so that we didn't have dead code hanging around.

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

Looks good, I left two small comments. Really excited to get this in!

@polyfractal
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@polyfractal polyfractal merged commit f05b831 into elastic:master Feb 20, 2020
polyfractal added a commit to polyfractal/elasticsearch that referenced this pull request Mar 30, 2020
elastic#52493)

This adds a test to AggregatorTestCase that allows us to programmatically
verify that an aggregator supports or does not support a particular
field type.  It fetches the list of registered field type parsers,
creates a MappedFieldType from the parser and then attempts to run
a basic agg against the field.

A supplied list of supported VSTypes are then compared against the
output (success or exception) and suceeds or fails the test accordingly.

Co-Authored-By: Mark Tozzi <[email protected]>
* Skip fields that are not aggregatable
polyfractal added a commit that referenced this pull request Mar 31, 2020
…nations (#54451)

* Comprehensively test supported/unsupported field type:agg combinations (#52493)

This adds a test to AggregatorTestCase that allows us to programmatically
verify that an aggregator supports or does not support a particular
field type.  It fetches the list of registered field type parsers,
creates a MappedFieldType from the parser and then attempts to run
a basic agg against the field.

A supplied list of supported VSTypes are then compared against the
output (success or exception) and suceeds or fails the test accordingly.

Co-Authored-By: Mark Tozzi <[email protected]>
* Skip fields that are not aggregatable

* Use newIndexSearcher() to avoid incompatible readers (#52723)

Lucene's `newSearcher()` can generate readers like ParallelCompositeReader
which we can't use.  We need to instead use our helper `newIndexSearcher`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants