-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Clamp family of functions #135822
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
Clamp family of functions #135822
Conversation
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
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've left some comments. Thank you, Pablo! I will do another detailed review.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/adsads
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/asdasd
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-clamp.csv-spec
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/ClampMax.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/ClampMax.java
Show resolved
Hide resolved
...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/Clamp.java
Outdated
Show resolved
Hide resolved
...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/Clamp.java
Outdated
Show resolved
Hide resolved
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.
It's getting closer. Thanks Pablo!
|
||
**Description** | ||
|
||
Returns clamps the values of all input samples clamped to have an upper limit of max. |
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.
nit: remove returns
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.
done
docs/reference/query-languages/esql/_snippets/functions/description/clamp_min.md
Show resolved
Hide resolved
.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/ClampMin.java
Outdated
Show resolved
Hide resolved
...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/Clamp.java
Outdated
Show resolved
Hide resolved
...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/Clamp.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-clamp.csv-spec
Show resolved
Hide resolved
looks like we do need some serialization functions: https://buildkite.com/elastic/elasticsearch-pull-request/builds/97622#0199c4c2-84d0-4364-9f05-0e4764c8d79d/98-3431 |
You can disable it here for the ClampTests: Line 503 in d0d4339
|
that's embarrassing. thanks nhat - working on that... |
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 left some minor comments; the comments for ClampMax also applies to ClampMin. The PR looks great; please address these before merging. Thanks, Pablo!
|
||
@Override | ||
public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) { | ||
// force datatype initialization |
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.
leftover comment?
} | ||
|
||
var field = children().get(0); | ||
var min = children().get(1); |
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.
nit: max?
} | ||
resolution = TypeResolutions.isType( | ||
min, | ||
t -> t.isNumeric() ? fieldDataType.isNumeric() : t.noText() == fieldDataType, |
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.
t.noText() == fieldDataType.noText()
var outputType = dataType(); | ||
|
||
var max = children().get(1); | ||
var maxF = outputType != max.dataType() |
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.
Maybe check the element type of both side to avoid the cast? PlannerUtils.toElementType(outputType) != PlannerUtils.toElementType(max.dataType())?
12.125 | 2024-05-10T00:07:19.000Z | ||
12.125 | 2024-05-10T00:17:39.000Z | ||
12.0 | 2024-05-10T00:08:08.000Z | ||
; |
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.
another ask - can you ask a test where min/max is another column instead of constant?
Let's make sure Nhat's comments are addressed, in a follow-up. It's a good idea to skip auto-submit. |
yes I will address on a follow-up! |
No worries, thanks Pablo! They're just minor comments. |
TODO