Skip to content

Aggregation support for Value Scripts that change types #54830

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 24, 2020
Merged

Aggregation support for Value Scripts that change types #54830

merged 12 commits into from
Apr 24, 2020

Conversation

@not-napoleon
Copy link
Member

@not-napoleon not-napoleon commented Apr 6, 2020

Resolves #54655 for post-refactor ValuesSourceConfig. Will need a slightly different solution to backport to 7.7.x

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 6, 2020

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

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 10, 2020

@elasticmachine run elasticsearch-ci/2

polyfractal
polyfractal reviewed Apr 13, 2020
View reviewed changes
Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but the test failure also looks real (NPE in stringstats "no matching docs" test), so it feels like we might be missing an edge-case here. Or maybe just a broken test that is being exposed due to the change :)

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 17, 2020

@polyfractal Neither, really. I used an Intellij refactoring to pull the generic TestCase method we use in a lot of places up into the AggregatorTestCase base class, rather than copy it again, but I hadn't noticed that StringStatsTests had different behavior in its implementation. I just pushed a fix that restores the old version of testcase in StringStatsTests with a TODO note to harmonize it with the base class version.

polyfractal
polyfractal approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

Ah yeah, we have a few of these hanging around, particularly older aggs. I just converted Terms over to use the base class with a similar pattern, and it requires a bit of reworking some of the tests.

👍

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 20, 2020

@elasticmachine update branch

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 20, 2020

@elasticmachine update branch

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 20, 2020

@elasticmachine update branch

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 22, 2020

@elasticmachine update branch

elasticmachine and others added 2 commits April 22, 2020 15:31
@elasticmachine
Merge branch 'master' into bugfix-mixed-type-values-scripts
b160c01
@not-napoleon
Merge branch 'master' into bugfix-mixed-type-values-scripts
ab9c2d7 
 Conflicts:
	test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java
@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 24, 2020

@elasticmachine run elasticsearch-ci/1

@not-napoleon
Copy link
Member Author

not-napoleon commented Apr 24, 2020

@elasticmachine update branch

@not-napoleon not-napoleon merged commit 754e3ca into elastic:master Apr 24, 2020
@not-napoleon not-napoleon deleted the bugfix-mixed-type-values-scripts branch April 24, 2020 18:45
@not-napoleon not-napoleon mentioned this pull request Apr 24, 2020
Merged
not-napoleon added a commit that referenced this pull request Apr 27, 2020
@not-napoleon
Aggregation support for Value Scripts that change types (#54830) (#55752
22a98ec 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@polyfractal polyfractal polyfractal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Analytics/Aggregations Aggregations >bug v7.8.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Value Scripts that change the field type are broken

5 participants

@not-napoleon @elasticmachine @polyfractal @jakelandis @jrodewig