Skip to content

Percentiles aggregation validation checks for range#51871

Merged
hendrikmuhs merged 6 commits intoelastic:masterfrom
hendrikmuhs:percentile-aggregation-checks
Feb 11, 2020
Merged

Percentiles aggregation validation checks for range#51871
hendrikmuhs merged 6 commits intoelastic:masterfrom
hendrikmuhs:percentile-aggregation-checks

Conversation

@hendrikmuhs
Copy link
Copy Markdown

@hendrikmuhs hendrikmuhs commented Feb 4, 2020 •
edited
Loading

disallow to specify percentile out of range [0,100]. This also fixes a problem in transform by failing validation if an invalid percentile configuration is used.

Note: The duplication check is a breaking change if you are picky (question is, if it can be found in the wild). -> duplication moved out to PR: TBD

For release notes: Please list it under aggs (percentiles in transforms is a new feature for 7.7.0)

relates to #51808

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 4, 2020

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

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 4, 2020

Pinging @elastic/ml-core (:ml/Transform)

@hendrikmuhs
Copy link
Copy Markdown
Author

hendrikmuhs commented Feb 5, 2020

run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro

@hendrikmuhs hendrikmuhs force-pushed the percentile-aggregation-checks branch from 41fe6b8 to 442fb22 Compare February 5, 2020 20:13
@benwtrent benwtrent removed the :ml/Transform Transform label Feb 6, 2020
@hendrikmuhs hendrikmuhs added the :ml/Transform Transform label Feb 6, 2020
@nik9000 nik9000 self-requested a review February 6, 2020 16:37
nik9000
nik9000 reviewed Feb 6, 2020
View reviewed changes
...c/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java Outdated
throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]");
}

if(percent == previousPercent) {
Copy link
Copy Markdown
Member

@nik9000 nik9000 Feb 6, 2020

Choose a reason for hiding this comment

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

I think this should have an extra space after if and before (.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Feb 6, 2020

Note: The duplication check is a breaking change if you are picky (question is, if it can be found in the wild).

I imagine it can happen, mostly in the case where folks build the percentiles with a tool. What does it look like when there are dups?

I certainly would feel more comfortable making the change in 8.0 only.

@hendrikmuhs
Copy link
Copy Markdown
Author

hendrikmuhs commented Feb 7, 2020

I imagine it can happen, mostly in the case where folks build the percentiles with a tool. What does it look like when there are dups?

  "aggregations" : {
    "percentile_field" : {
      "values" : {
        "1.0" : 0.0,
        "5.0" : 274.3552087114338,
        "25.0" : 3009.2182284054206,
        "50.0" : 5480.849716847411,
        "50.0" : 5480.849716847411,
        "75.0" : 8015.768454300776,
        "95.0" : 10010.003716690042,
        "99.0" : 17937.91377777778
      }
    }
  }

It depends on the json parser to accept this, it seems common behavior to silently remove the double entry and take the last value.

This issue exists now for a long time without no one noticed or at least not finding it problematic. I therefore agree we can wait for 8.0 to fix it.

I certainly would feel more comfortable making the change in 8.0 only.

I can split the PR:

  • the range validation is important for transforms and required for 7.7, this is a non-breaking change.
  • for the double percentiles output I added a workaround in [Transform] add support for percentile aggs #51808, so it won't break for transforms, I can move that part into a separate 8.0 only PR

@hendrikmuhs hendrikmuhs changed the title Percentiles aggregation validation checks for range and dups Percentiles aggregation validation checks for range Feb 7, 2020
@hendrikmuhs
Copy link
Copy Markdown
Author

hendrikmuhs commented Feb 7, 2020

I removed the duplication check. I will create a separate PR for this, 8.0 only.
However I require the changes from this PR, so I will do it after this has been merged.

nik9000
nik9000 approved these changes Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

@hendrikmuhs hendrikmuhs merged commit 8b9e18e into elastic:master Feb 11, 2020
@hendrikmuhs hendrikmuhs deleted the percentile-aggregation-checks branch February 11, 2020 16:24
hendrikmuhs pushed a commit that referenced this pull request Feb 11, 2020
Percentiles aggregation validation checks for range (#51871)
098380e 
disallow to specify percentile out of range [0,100]. This also fixes a problem in transform by failing
validation if an invalid percentile configuration is used.
@hendrikmuhs hendrikmuhs mentioned this pull request Feb 12, 2020
Merged
hendrikmuhs pushed a commit that referenced this pull request Feb 26, 2020
Percentiles aggregation: disallow specifying same percentile v… (#52257)
854f698 
Disallow specifying the same percentile multiple times in percentiles
aggregation

Related: #51871
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
@hkulekci hkulekci mentioned this pull request Feb 17, 2022
Merged
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees

No one assigned

Labels

:Analytics/Aggregations Aggregations >enhancement :ml/Transform Transform v7.7.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hendrikmuhs @elasticmachine @nik9000 @jakelandis @benwtrent