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.

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

ESQL: Added Median and MedianAbsoluteDeviation aggregations tests and kibana docs #111231

Merged
merged 10 commits into from
Jul 26, 2024
Merged

ESQL: Added Median and MedianAbsoluteDeviation aggregations tests and kibana docs #111231

merged 10 commits into from
Jul 26, 2024

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Jul 24, 2024

No description provided.

@ivancea ivancea added >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI labels Jul 24, 2024
@ivancea ivancea requested review from nik9000 and not-napoleon July 24, 2024 10:47
@github-actions GitHub Actions
Copy link
Contributor

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/kibana-esql (ES|QL-ui)

@ivancea ivancea changed the title ESQL: Added Median aggregation tests and kibana docs ESQL: Added Median and MedianAbsoluteDeviation aggregations tests and kibana docs Jul 24, 2024
@ivancea ivancea mentioned this pull request Jul 24, 2024
Closed
nik9000
nik9000 approved these changes Jul 24, 2024
View reviewed changes
...java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianAbsoluteDeviation.java
returnType = { "double", "integer", "long" },
description = "The median absolute deviation, a measure of variability.",
isAggregation = true
returnType = "double",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@nik9000
Copy link
Member

nik9000 commented Jul 24, 2024

Hmmmm.... Maybe we're duplicating something.
image

not-napoleon
not-napoleon approved these changes Jul 24, 2024
View reviewed changes
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.

LGTM

@ivancea
Copy link
Contributor Author

ivancea commented Jul 24, 2024

Hmmmm.... Maybe we're duplicating something. image

@nik9000 Yeah, not sure about that one. Happens in multiple aggs (That screenshot is from the old docs), so for now I did the same for them all: Move the warning part to the "appendix" (At the end).

I'm not sure it's duplicated tho, as one talks about approximation and the other about determinism. We could merge them in the notes, or move both to a warning. As I don't have a strong opinion about which one, I just kept them that way for now (?)

@nik9000
Copy link
Member

nik9000 commented Jul 24, 2024

I think we should probably try for one WARNING. Something like "MAD is approximate and non-deterministic."

@ivancea ivancea added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 26, 2024
ivancea added 2 commits July 26, 2024 10:31
@ivancea
Merge branch 'main' into median-agg-tests
695843c
@ivancea
Merge branch 'main' into median-agg-tests
5e20acf 
# Conflicts:
#	docs/reference/esql/functions/aggregation-functions.asciidoc
@elasticsearchmachine elasticsearchmachine merged commit 826d494 into elastic:main Jul 26, 2024
15 checks passed
@ivancea ivancea deleted the median-agg-tests branch July 26, 2024 12:11
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

@not-napoleon not-napoleon not-napoleon approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) ES|QL-ui Impacts ES|QL UI Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivancea @elasticsearchmachine @nik9000 @not-napoleon