ES|QL change_point processing command #120998
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elasticsearchmachine
added
needs:triage
jan-elastic
added
>feature
:ml
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @jan-elastic, I've created a changelog YAML for you. |
jan-elastic
force-pushed
the
esql-changepoint-processing-command
branch
from
9004e67 to
e905a79
Compare
…ngepoint-processing-command
alex-spies
approved these changes
Feb 5, 2025
There was a problem hiding this comment.
Ok, I have now had a thorough look at everything except the operator implementation and associated tests. But seems like @nik9000 had a look at that.
Everything I saw looked super nice; I left very minor notes, only, which in some cases are more relevant for follow-ups anyway. Thumbs up, LGTM!
x-pack/plugin/esql/qa/testFixtures/src/main/resources/change_point.csv-spec
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/change_point.csv-spec
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/change_point.csv-spec
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/ChangePoint.java
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/change_point.csv-spec
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Show resolved
Hide resolved
nik9000
approved these changes
Feb 5, 2025
…ngepoint-processing-command
This reverts commit 21ec77c.
alex-spies
approved these changes
Feb 7, 2025
There was a problem hiding this comment.
Thanks for the iterations @jan-elastic , I think this is in a very nice state!
I have some final observations, but please consider them at your own discretion and feel free to merge this :) 🎉
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/ChangePoint.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/change_point.csv-spec
Outdated
Show resolved
Hide resolved
alex-spies
approved these changes
Feb 10, 2025
There was a problem hiding this comment.
Nice, the MV handling being consistent with other MV handling in ESQL is great - thanks for handling that!
This PR looks very nice and should be merged IMO :)
My only nit is that now that the change point operator does handle quite a few cases (nulls, mvs, different data types...), we could try to have more coverage in the change point operator tests. The usual way to achieve this is via randomization, but that may be tough to implement nicely. In any case, that's a matter for another PR if anything. Let's 
!
4 tasks
jan-elastic
added a commit
that referenced
this pull request
Apr 14, 2025
* Grammar for ES|QL change point (with dummy impl) * pipeline breaker * ChangePointOperator * Add sorting * basic csv test * conflict * Update docs/changelog/120998.yaml * [CI] Auto commit changes from spotless * polish * Non-long data type * Move OrderBy/Limit to the logical plan * fix mem.leak * csv test for reusing column names * Warning indeterminable * capability * handle null values * too much data * type text->keyword * default timestamp and output columns * spotless * ChangePointOperatorTests + fix memory leaks * [CI] Auto commit changes from spotless * improve test * add comments/todos * handle multivalued columns * don't register unserialiazable * surrogate * make "too much data" tests readable * more tests * Error handling * fix multivalued test * more name conflict tests * [CI] Auto commit changes from spotless * more tests * improve code * CSV test for various input key/value types * one more csv test * Check sortable/numeric for all types * add null type to testChangePoint_valueNumeric * more CSV tests * skip nulls instead of zeroing them * error on MV * Test+todo for nicer error message * better error msg * Revert "better error msg" This reverts commit 21ec77c. * fix * make csv test deterministic * replace NamedExpression -> Attribute * skip MVs + warning --------- Co-authored-by: elasticsearchmachine <[email protected]>
jan-elastic
added a commit
that referenced
this pull request
Apr 15, 2025
* ES|QL change_point processing command (#120998) * Grammar for ES|QL change point (with dummy impl) * pipeline breaker * ChangePointOperator * Add sorting * basic csv test * conflict * Update docs/changelog/120998.yaml * [CI] Auto commit changes from spotless * polish * Non-long data type * Move OrderBy/Limit to the logical plan * fix mem.leak * csv test for reusing column names * Warning indeterminable * capability * handle null values * too much data * type text->keyword * default timestamp and output columns * spotless * ChangePointOperatorTests + fix memory leaks * [CI] Auto commit changes from spotless * improve test * add comments/todos * handle multivalued columns * don't register unserialiazable * surrogate * make "too much data" tests readable * more tests * Error handling * fix multivalued test * more name conflict tests * [CI] Auto commit changes from spotless * more tests * improve code * CSV test for various input key/value types * one more csv test * Check sortable/numeric for all types * add null type to testChangePoint_valueNumeric * more CSV tests * skip nulls instead of zeroing them * error on MV * Test+todo for nicer error message * better error msg * Revert "better error msg" This reverts commit 21ec77c. * fix * make csv test deterministic * replace NamedExpression -> Attribute * skip MVs + warning --------- Co-authored-by: elasticsearchmachine <[email protected]> * Move ES|QL change_point to tech preview * fix grammar * License check * docs + example * spotless * fix compile error * add change_point to ES|QL processing commands overview page * close note * close note * fix link --------- Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Craig Taverner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.