Add AD command for PPL/AD integration #455

jackiehanyang · 2022-03-03T21:16:28Z

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --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.

Signed-off-by: jackieyanghan <[email protected]>

codecov-commenter · 2022-03-03T21:25:18Z

Codecov Report

Merging #455 (fa1ef16) into feature/ppl-ml (9768bd9) will decrease coverage by 0.57%.
The diff coverage is 29.60%.

@@                 Coverage Diff                  @@
##             feature/ppl-ml     #455      +/-   ##
====================================================
- Coverage             95.78%   95.20%   -0.58%     
- Complexity             2704     2715      +11     
====================================================
  Files                   269      273       +4     
  Lines                  7281     7364      +83     
  Branches                544      550       +6     
====================================================
+ Hits                   6974     7011      +37     
- Misses                  252      298      +46     
  Partials                 55       55

Flag	Coverage Δ
query-workbench	`62.91% <ø> (ø)`
sql-engine	`98.37% <29.60%> (-0.68%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...a/org/opensearch/sql/utils/MLCommonsConstants.java	`0.00% <0.00%> (ø)`
...ch/sql/opensearch/planner/physical/ADOperator.java	`0.00% <0.00%> (ø)`
...opensearch/planner/physical/MLCommonsOperator.java	`30.43% <0.00%> (+18.95%)`	⬆️
...rch/planner/physical/MLCommonsOperatorActions.java	`1.75% <1.75%> (ø)`
...ain/java/org/opensearch/sql/analysis/Analyzer.java	`100.00% <100.00%> (ø)`
.../org/opensearch/sql/planner/logical/LogicalAD.java	`100.00% <100.00%> (ø)`
...ch/sql/planner/logical/LogicalPlanNodeVisitor.java	`100.00% <100.00%> (ø)`
.../sql/planner/physical/PhysicalPlanNodeVisitor.java	`100.00% <100.00%> (ø)`
...ecutor/protector/OpenSearchExecutionProtector.java	`100.00% <100.00%> (ø)`
...search/sql/opensearch/storage/OpenSearchIndex.java	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9768bd9...fa1ef16. Read the comment docs.

penghuo · 2022-03-04T19:29:12Z

ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java

+ return new HashMap<String, Literal>() {{
+ put("shingle_size", (ctx.shingle_size != null)
+ ? getArgumentValue(ctx.shingle_size)
+ : new Literal(8, DataType.INTEGER));


If shingle_size = 8 is default setting, it would be better to encapsulated in AD operator instead of in parser.

Shingle size is optional, MLCommons will set it as 8 by default https:/opensearch-project/ml-commons/blob/main/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/rcf/FixedInTimeRandomCutForest.java#L82

Changed to not set default value here in the latest revision

penghuo · 2022-03-04T19:30:56Z

core/src/main/java/org/opensearch/sql/data/model/ExprBooleanValue.java

@@ -19,7 +19,7 @@

 private final Boolean value;

- private ExprBooleanValue(Boolean value) {
+ public ExprBooleanValue(Boolean value) {


why change to public?

Need to reference it in resultBuilder.put(resultKeyName, new ExprBooleanValue(columnValue.booleanValue()));. Other Expr value classes under this path all have public access.

use ExprBooleanValue.of() method instead.

penghuo · 2022-03-04T19:36:28Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java

+ return resultBuilder.build();
+ }
+
+ private void popluateResultBuilder(ColumnValue columnValue,


Is it duplicated with convertRowIntoExprValue() in MLCommonsOperator?

I think we can consider abstract such common shared code, that may benefit general train , predict and train_predict command

Abstract duplicate code in the latest revision

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java

penghuo · 2022-03-04T19:47:28Z

In general,

Please add more detail description in PR.
Please add docs for new commands. It is not only docs also used as integration test. https:/opensearch-project/sql/blob/main/docs/dev/Doctest.md

ylwu-amzn · 2022-03-04T19:59:58Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java

+ }
+
+ protected MLAlgoParams convertArgumentToMLParameter(Map<String, Literal> arguments) {
+ if (arguments.get("time_field").getValue() == null) {


See multiple places using "time_field". How about creating constants and reuse?

Couldn't find any Constants class to use at first in this package, so didn't use constants to represent these fields. Created a constant class in the latest revision to store MLCommons related constants.

ylwu-amzn · 2022-03-04T20:04:08Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java

+ // change key name to avoid duplicate key issue in result map
+ // only value will be shown in the final returned result
+ if (schema.containsKey(resultKeyName)) {
+ resultKeyName = resultKeyName + "1";


Is it general name convention in SQL to append "1" to avoid duplicate name? Seems not so readable, maybe we can append algorithm/command name like "_ad", or "_batch_rcf"?

Not sure what's the best practice to address this, need to discuss it with package owner to find out. All the other command arguments are built into a list, instead of a map that we used here. So no similar case to reference.

ylwu-amzn · 2022-03-04T20:06:11Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

@@ -89,6 +89,13 @@ kmeansCommand
 k=integerLiteral
 ;

+adCommand
+ : AD


We should support the other parameters. You can do it in next PR.

Opened an issue to track it - #462
Will address this in the PR.

Signed-off-by: jackieyanghan <[email protected]>

ylwu-amzn

LGTM

penghuo · 2022-03-08T18:47:27Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/OperatorActions.java

+import org.opensearch.sql.opensearch.client.MLClient;
+import org.opensearch.sql.planner.physical.PhysicalPlan;
+
+public abstract class OperatorActions extends PhysicalPlan {


OperatorActions is too generic. Could you make more specific for ML.

By the way, add java docs for abstract class and public/abstract methods.

Signed-off-by: jackieyanghan <[email protected]>

penghuo

LGTM

jackiehanyang added 5 commits March 3, 2022 10:54

PPL AD integration

5f3b7e2

Signed-off-by: jackieyanghan <[email protected]>

Update AD command format

f9cdb0b

Signed-off-by: jackieyanghan <[email protected]>

Address issues with ad command

7dc1b45

Signed-off-by: jackieyanghan <[email protected]>

Separate fit_rcf schema and batch_rcf schema

2a41e81

Signed-off-by: jackieyanghan <[email protected]>

Add tests for AD command

96f5ba9

Signed-off-by: jackieyanghan <[email protected]>

jackiehanyang requested a review from a team as a code owner March 3, 2022 21:16

penghuo reviewed Mar 4, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java Outdated Show resolved Hide resolved

penghuo reviewed Mar 4, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/ADOperator.java Outdated Show resolved Hide resolved

ylwu-amzn reviewed Mar 4, 2022

View reviewed changes

jackiehanyang added 2 commits March 4, 2022 13:27

Abstract duplicate code in ADOperator and MLCommonsOperator

192537e

Signed-off-by: jackieyanghan <[email protected]>

Update dependency opensearch-ml-client version

54660d8

Signed-off-by: jackieyanghan <[email protected]>

jackiehanyang requested a review from penghuo March 7, 2022 22:44

ylwu-amzn approved these changes Mar 8, 2022

View reviewed changes

penghuo reviewed Mar 8, 2022

View reviewed changes

Add java doc on MLCommonsOperatorActions abstract class

fa1ef16

Signed-off-by: jackieyanghan <[email protected]>

penghuo approved these changes Mar 8, 2022

View reviewed changes

penghuo merged commit f042860 into opensearch-project:feature/ppl-ml Mar 8, 2022

jackiehanyang mentioned this pull request Mar 8, 2022

PPL integration with AD and ml-commons #468

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AD command for PPL/AD integration #455

Add AD command for PPL/AD integration #455

jackiehanyang commented Mar 3, 2022

codecov-commenter commented Mar 3, 2022 •

edited

Loading

penghuo Mar 4, 2022

ylwu-amzn Mar 4, 2022

jackiehanyang Mar 4, 2022

penghuo Mar 4, 2022

jackiehanyang Mar 4, 2022 •

edited

Loading

penghuo Mar 8, 2022

penghuo Mar 4, 2022

ylwu-amzn Mar 4, 2022

jackiehanyang Mar 4, 2022

penghuo commented Mar 4, 2022

ylwu-amzn Mar 4, 2022

jackiehanyang Mar 4, 2022

ylwu-amzn Mar 4, 2022 •

edited

Loading

jackiehanyang Mar 4, 2022

ylwu-amzn Mar 4, 2022

jackiehanyang Mar 4, 2022

ylwu-amzn left a comment

penghuo Mar 8, 2022

penghuo Mar 8, 2022

penghuo left a comment

Add AD command for PPL/AD integration #455

Add AD command for PPL/AD integration #455

Conversation

jackiehanyang commented Mar 3, 2022

Description

Issues Resolved

Check List

codecov-commenter commented Mar 3, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackiehanyang Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 3, 2022 •

edited

Loading

jackiehanyang Mar 4, 2022 •

edited

Loading

ylwu-amzn Mar 4, 2022 •

edited

Loading