-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Implementing copy_sign function for ESQL #128281
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
Conversation
7c63288 to
2f91dad
Compare
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CopySign.java
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CopySign.java
Outdated
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CopySign.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml
Outdated
Show resolved
Hide resolved
2f91dad to
7e053c9
Compare
Once you've opened the PR try not to force push to it. We squash and rebase on merge so the commits don't make it into the main branch. And reviewers can use the commits to follow along. Don't worry about crafting them carefully, but it is useful to have them. |
60d7b63 to
c80bd11
Compare
ah yes my bad! I was force-pushing my initial changes (before any review) - I'll avoid it in the future |
Eh. It's no big deal. Sometimes you have to. But I tend to never force push any PRs so I never have to worry about if someone is looking yet. Just less stuff for me to even remember. |
c80bd11 to
5133010
Compare
4ff9932 to
ad9ecc6
Compare
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec
Outdated
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CopySign.java
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CopySign.java
Show resolved
Hide resolved
nik9000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I withdraw my main question. I requested a few comments and formatting things, but feel free to merge as soon as you think you've done enough there. No need to wait on me unless you think one of my requests is way off base.
| import java.util.Map; | ||
|
|
||
| /** | ||
| * ES|QL function that mimics the behavior of Math.copySign(double magnitude, double sign). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use {@code Math.copySign(double magnitude, double sign)} for make this render nicely on mouseover and in the actual generated docs. No one reads those. But we do read the mouseovers.
Implementing copy_sign