-
Notifications
You must be signed in to change notification settings - Fork 33
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
New trendline ppl command (WMA) #872
New trendline ppl command (WMA) #872
Conversation
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.
@andy-k-improving
please add the relevant documentation references including examples
@andy-k-improving please add DCO (sign-off) |
...est/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLTrendlineITSuite.scala
Outdated
Show resolved
Hide resolved
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Outdated
Show resolved
Hide resolved
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Outdated
Show resolved
Hide resolved
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Show resolved
Hide resolved
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Show resolved
Hide resolved
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Show resolved
Hide resolved
b590fa8
to
ec05bf6
Compare
Done. |
02c517e
to
97317b5
Compare
Done. |
@YANG-DB I have updated the example and documentation, would you mind to have another look? Thanks, |
Hi @dai-chen , @LantaoJin and @salyh , would you guys mind to have look on this, any feedback would be appreciated :) |
e5eeeda
to
8f4efd5
Compare
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.
@andy-k-improving this looks good - only a few questions commented
ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/utils/TrendlineCatalystUtils.java
Show resolved
Hide resolved
@@ -283,6 +283,7 @@ public enum BuiltinFunctionName { | |||
WILDCARDQUERY(FunctionName.of("wildcardquery")), | |||
WILDCARD_QUERY(FunctionName.of("wildcard_query")), | |||
|
|||
NTH_VALUE(FunctionName.of("nth_value")), |
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.
This class is for PPL builtin function, not Spark. If we want to add a new PPL builtin function, NTH_VALUE
should be added in Lexer and Parser, and user doc too. Or remove this line to avoid confusion.
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.
Let's remove it, for now I'm good with using nth_value
String Literal directly as only WMA using this.
The CI failure caused by #903 is not related but blocks your whole testing process. @andy-k-improving could you merge the latest code from main? |
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
Signed-off-by: Andy Kwok <[email protected]>
a9e9aaf
to
2b61f4c
Compare
Thanks for your contribution. Merging to main. |
* WMA implementation Signed-off-by: Andy Kwok <[email protected]> * Update test cases Signed-off-by: Andy Kwok <[email protected]> * Update tests Signed-off-by: Andy Kwok <[email protected]> * Refactor code Signed-off-by: Andy Kwok <[email protected]> * Addres comments Signed-off-by: Andy Kwok <[email protected]> * Update doc Signed-off-by: Andy Kwok <[email protected]> * Update example Signed-off-by: Andy Kwok <[email protected]> * Update readme Signed-off-by: Andy Kwok <[email protected]> * Update scalafmt Signed-off-by: Andy Kwok <[email protected]> * Update grammar rule Signed-off-by: Andy Kwok <[email protected]> * Address review comments Signed-off-by: Andy Kwok <[email protected]> * Address review comments Signed-off-by: Andy Kwok <[email protected]> --------- Signed-off-by: Andy Kwok <[email protected]> (cherry picked from commit 439cf3e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* WMA implementation * Update test cases * Update tests * Refactor code * Addres comments * Update doc * Update example * Update readme * Update scalafmt * Update grammar rule * Address review comments * Address review comments --------- (cherry picked from commit 439cf3e) Signed-off-by: Andy Kwok <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* WMA implementation Signed-off-by: Andy Kwok <[email protected]> * Update test cases Signed-off-by: Andy Kwok <[email protected]> * Update tests Signed-off-by: Andy Kwok <[email protected]> * Refactor code Signed-off-by: Andy Kwok <[email protected]> * Addres comments Signed-off-by: Andy Kwok <[email protected]> * Update doc Signed-off-by: Andy Kwok <[email protected]> * Update example Signed-off-by: Andy Kwok <[email protected]> * Update readme Signed-off-by: Andy Kwok <[email protected]> * Update scalafmt Signed-off-by: Andy Kwok <[email protected]> * Update grammar rule Signed-off-by: Andy Kwok <[email protected]> * Address review comments Signed-off-by: Andy Kwok <[email protected]> * Address review comments Signed-off-by: Andy Kwok <[email protected]> --------- Signed-off-by: Andy Kwok <[email protected]>
Description
Introduce a new variant (WMA) for existing trendline ppl command, by compositing a logical plan similar to the following with function
nth_value( )
to calculate the WMA value by perform event look behind.Some high level code changes:
TrendLine
processing logic, as sort field is mandatory for WMA calculation.TrendlineCatalystUtils.java
to have a new code path for WMA selection and associated calculation logic.Related Issues
Prior implement for SMA formula: #833
Check List
--signoff
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Test plan:
Despite the existing unit test / integration test, the feature can also be tested manually, by first inserting a simple table, then run PPL trend line command against the table to calculate WMA value.