Sqrt function for ESQL#98449
Conversation
Introduces a unary scalar function for square root, which is a thin wrapper over the Java.Math implementation.
|
Documentation preview: |
|
@elasticsearchmachine update branch |
|
@elasticsearchmachine run elasticsearch-ci/part-3 |
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
|
@elasticsearchmachine run elasticsearch-ci/docs-check |
|
@elasticsearchmachine run elasticsearch-ci/part-1 |
|
@elasticsearchmachine create changelog |
|
@elasticsearchmachine generate changelog |
bpintea
left a comment
There was a problem hiding this comment.
Apart from the missing unsigned long support, it LGTM.
| return () -> new SqrtLongEvaluator(eval); | ||
| } | ||
|
|
||
| throw new UnsupportedOperationException("Unsupported type " + fieldType); |
There was a problem hiding this comment.
The UNSIGNED_LONG support would need to be added too. OK if added in a subsequent PR.
There was a problem hiding this comment.
What's the suggested approach for handling it? Should it be handled as a long if less than Long. MAX_VALUE, otherwise as a double?
Since the rest of scalar functions don't handle it either, they can all be updated in a separate PR?
There was a problem hiding this comment.
I'd delay it, yeah. @bpintea maybe we need a meta issue to track missing functions for unsigned long. I imagine we have a few. And they are always kind of tricky to write.
There was a problem hiding this comment.
the rest of scalar functions don't handle it either
@kkrik-es which functions do you refer to? I was hoping I covered them all, but that might not be the case and I'll open an issue for those.
What's the suggested approach for handling it?
I think it'll eventually be fed to Math.sqrt() as a double, so converting to double should be fine imo.
There was a problem hiding this comment.
Right, that makes sense.
Wrt other functions, I see Log10 that I used as a template.
There was a problem hiding this comment.
@kkrik-es nice, yes, that's a bug.
I did a quick pass and I hope I didn't miss any other numeric function not supporting UL right now. EDIT: some aggs still need some support, so opened #98544.
If you want to have a look at both functions (i.e. this one and log10, which I think would be pretty similar) feel free to, otherwise I can have a look into log10 as well.
There was a problem hiding this comment.
I'm happy to take care of these two. I'll probably wait for merging to branch to main (if it hasn't happened) to avoid duplicate PRs.
* Sqrt function for ESQL Introduces a unary scalar function for square root, which is a thin wrapper over the Java.Math implementation. * Fix area for ESQL integration changelog. * Restore changelog. * Restore area in changelog.
Introduces a unary scalar function for square root, which is a
thin wrapper over the Java.Math implementation.