Skip to content

[ES|QL] Add hypot function #114382

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 5 commits into from
Oct 11, 2024
Merged

[ES|QL] Add hypot function #114382

merged 5 commits into from
Oct 11, 2024

Conversation

limotova
Copy link
Contributor

@limotova limotova commented Oct 9, 2024

Adds a hypotenuse function

@limotova
[ES|QL] Add hypot function
03ff460 
Adds a hypotenuse function
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 9, 2024

Documentation preview:

dnhatn
dnhatn approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

Changes look great. Thank you, Larisa! I'd like someone from the ES|QL team to take another look.

@dnhatn dnhatn added the auto-backport Automatically create backport pull requests when merged label Oct 9, 2024
@dnhatn dnhatn marked this pull request as ready for review October 9, 2024 23:50
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 9, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

ivancea
ivancea approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@ivancea ivancea 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!

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec
3.0 | 4.0 | 5.0
// end::hypot-result[]
;

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add some extra cases:

  • Having null in one and both parameters
  • Another one using FROM instead of ROW, as they work differently (ROW may get folded as it's all constants, for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some tests..! I wasn't able to find any sample data with numbers specifically that would make more sense to calculate the hypotenuse of, so I hope it's alright that some of them are a little nonsensical

Copy link
Member

Choose a reason for hiding this comment

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

employees also has some numeric data types - integer, double, long etc., if you are looking for data types besides unsigned long. They are usable, although a little nonsensical too.

Copy link
Contributor

Choose a reason for hiding this comment

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

++. It doesn't really matter if they make sense IMO, as long as they come from an index. For interesting values, there's already the function tests

@limotova limotova requested a review from ivancea October 11, 2024 01:34
fang-xing-esql
fang-xing-esql approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@limotova limotova merged commit 2155f1b into elastic:main Oct 11, 2024
16 checks passed
@limotova limotova deleted the add-hypot-function branch October 11, 2024 19:34
@limotova limotova mentioned this pull request Oct 11, 2024
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.x

limotova added a commit to limotova/elasticsearch that referenced this pull request Oct 11, 2024
@limotova
[ES|QL] Add hypot function (elastic#114382)
764339e 
Adds a hypotenuse function
elasticsearchmachine pushed a commit that referenced this pull request Oct 11, 2024
@limotova
[ES|QL] Add hypot function (#114382) (#114658)
42aa343 
Adds a hypotenuse function
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Oct 13, 2024
@limotova @davidkyle
[ES|QL] Add hypot function (elastic#114382)
a4b78ee 
Adds a hypotenuse function
@ioanatia ioanatia mentioned this pull request Oct 14, 2024
Merged
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024
@limotova @georgewallace
[ES|QL] Add hypot function (elastic#114382)
b95bf7d 
Adds a hypotenuse function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivancea ivancea ivancea approved these changes

@dnhatn dnhatn dnhatn approved these changes

@fang-xing-esql fang-xing-esql fang-xing-esql approved these changes

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >enhancement ES|QL-ui Impacts ES|QL UI Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.16.0 v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@limotova @elasticsearchmachine @ivancea @dnhatn @fang-xing-esql