A Generic ML Command in PPL #971

jngz-es · 2022-10-26T19:22:30Z

Description

A generic ml command in ppl to apply algorithms in ml-commons.

Issues Resolved

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: Jing Zhang <[email protected]>

codecov-commenter · 2022-10-26T19:43:42Z

Codecov Report

Merging #971 (f2bfe22) into 2.x (634e2ff) will decrease coverage by 34.84%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##                2.x     #971       +/-   ##
=============================================
- Coverage     97.60%   62.76%   -34.85%     
=============================================
  Files           308       10      -298     
  Lines          7983      658     -7325     
  Branches        520      119      -401     
=============================================
- Hits           7792      413     -7379     
- Misses          190      192        +2     
- Partials          1       53       +52

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`?`

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

Impacted Files	Coverage Δ
...ain/java/org/opensearch/sql/analysis/Analyzer.java
...a/org/opensearch/sql/analysis/TypeEnvironment.java
...ch/sql/planner/logical/LogicalPlanNodeVisitor.java
.../sql/planner/physical/PhysicalPlanNodeVisitor.java
...a/org/opensearch/sql/utils/MLCommonsConstants.java
...ecutor/protector/OpenSearchExecutionProtector.java
...rch/planner/physical/MLCommonsOperatorActions.java
...search/sql/opensearch/storage/OpenSearchIndex.java
...java/org/opensearch/sql/ppl/parser/AstBuilder.java
...l/opensearch/data/value/OpenSearchExprIpValue.java
... and 308 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

penghuo

running git checkout sql-jdbc/docs/img/tableau_graph.PNG restore deleted file. reference. #865 (comment)

penghuo · 2022-10-27T02:40:40Z

ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java

@@ -411,6 +412,20 @@ public UnresolvedPlan visitAdCommand(AdCommandContext ctx) {
    return new AD(builder.build());
  }

+  /**
+   * Kmeans command.


correct comments.

Good catch, will correct it to ml.

penghuo · 2022-10-27T02:42:33Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

@@ -148,6 +148,14 @@ adParameter
    | (ANOMALY_SCORE_THRESHOLD EQUAL anomaly_score_threshold=decimalLiteral)
    ;

+mlCommand


does ml also no arguments? e.g. source = index | ml. The syntax actaully allow this.

If no action or algorithm arguments, it will throw exception. I don't think we have to restrict the command content at command parsing stage, we leave all parameters validation to ml-client.

penghuo · 2022-10-27T02:44:23Z

core/src/main/java/org/opensearch/sql/planner/logical/LogicalML.java

+@Getter
+@ToString
+@EqualsAndHashCode(callSuper = true)
+public class LogicalML extends LogicalPlan {


Do you plan deprecated Kmeans/AD in Logical plan?

Yes, we will deprecate them in the following version like 2.5, then remove them in 3.0.

penghuo · 2022-10-27T02:49:01Z

core/src/main/java/org/opensearch/sql/ast/tree/ML.java

+  public Map<String, ExprCoreType> getOutputSchema(TypeEnvironment env) {
+    switch (getAction()) {
+      case TRAIN:
+        env.clearAllFields();


why clean all fields?

The ml train command will return only model/task id, and status, so remove all fields from input fields.

penghuo · 2022-10-27T02:59:30Z

core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java

+
+    LogicalPlan actual = analyze(AstDSL.project(
+            new ML(AstDSL.relation("schema"), argumentMap), AstDSL.allFields()));
+    assertTrue(((LogicalProject) actual).getProjectList().size() >= 2);


why >= 2, does the result non deterministic?

It is deterministic, just at least 2.

penghuo · 2022-10-27T03:04:31Z

...h/src/main/java/org/opensearch/sql/opensearch/planner/physical/MLCommonsOperatorActions.java

+   * @param nodeClient node client
+   * @return ml-commons result
+   */
+  protected MLOutput getMLOutput(DataFrame inputDataFrame,


could also deprecated getMLPredictionResult if it is not required?

It is required for now, we can remove it when removing kmeans and ad commands.

penghuo · 2022-10-27T03:06:37Z

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

+ */
+@RequiredArgsConstructor
+@EqualsAndHashCode(callSuper = false)
+public class MLOperator extends MLCommonsOperatorActions {


what is difference with MLCommonsOperator, deptecated MLCommonsOperator?

MLOperator is totally new operator for all ml algorithms from ml-commons. I don't want to mix it with old operator as there is a big gap between them, and also for smoothly deprecating old operator in the future.

jngz-es · 2022-10-28T00:08:54Z

Didn' t realized it, will recover it.

Signed-off-by: Jing Zhang <[email protected]>

penghuo

Thanks for the change!
Please add ML commands related doc. And add deprecated describtion for AD and Kmeans command.

* Add generic ml command in ppl. Signed-off-by: Jing Zhang <[email protected]> * Recover ml client dependency. Signed-off-by: Jing Zhang <[email protected]> * Address the comments I. Signed-off-by: Jing Zhang <[email protected]> Signed-off-by: Jing Zhang <[email protected]> (cherry picked from commit c6b234c)

* Address the comments I. Signed-off-by: Jing Zhang <[email protected]> Signed-off-by: Jing Zhang <[email protected]> (cherry picked from commit c6b234c) Co-authored-by: Jing Zhang <[email protected]>

jngz-es added 2 commits October 25, 2022 19:53

Add generic ml command in ppl.

5f700d8

Signed-off-by: Jing Zhang <[email protected]>

Recover ml client dependency.

b67faaf

Signed-off-by: Jing Zhang <[email protected]>

jngz-es requested a review from a team as a code owner October 26, 2022 19:22

penghuo added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 26, 2022

penghuo requested changes Oct 27, 2022

View reviewed changes

penghuo reviewed Oct 27, 2022

View reviewed changes

dai-chen added ml Issues related to integration with ML commons and plugin PPL Piped processing language labels Oct 27, 2022

jngz-es added 2 commits October 28, 2022 13:28

Address the comments I.

0c413af

Signed-off-by: Jing Zhang <[email protected]>

Merge branch '2.x' into ml

2226a15

jngz-es requested a review from penghuo October 28, 2022 20:52

Merge branch '2.x' into ml

f2bfe22

penghuo approved these changes Oct 31, 2022

View reviewed changes

dai-chen approved these changes Oct 31, 2022

View reviewed changes

dai-chen merged commit c6b234c into opensearch-project:2.x Oct 31, 2022

dai-chen added the backport 2.4 label Oct 31, 2022

opensearch-trigger-bot bot mentioned this pull request Oct 31, 2022

[Backport 2.4] A Generic ML Command in PPL #997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A Generic ML Command in PPL #971

A Generic ML Command in PPL #971

jngz-es commented Oct 26, 2022

codecov-commenter commented Oct 26, 2022 •

edited

Loading

penghuo left a comment

penghuo Oct 27, 2022

jngz-es Oct 27, 2022

penghuo Oct 27, 2022

jngz-es Oct 27, 2022

penghuo Oct 27, 2022

jngz-es Oct 27, 2022

penghuo Oct 27, 2022

jngz-es Oct 27, 2022

penghuo Oct 27, 2022

jngz-es Oct 28, 2022

penghuo Oct 27, 2022

jngz-es Oct 28, 2022

penghuo Oct 27, 2022

jngz-es Oct 28, 2022

jngz-es commented Oct 28, 2022

penghuo left a comment

A Generic ML Command in PPL #971

A Generic ML Command in PPL #971

Conversation

jngz-es commented Oct 26, 2022

Description

Issues Resolved

Check List

codecov-commenter commented Oct 26, 2022 • edited Loading

Codecov Report

penghuo left a 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

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

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

Choose a reason for hiding this comment

jngz-es commented Oct 28, 2022

penghuo left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 26, 2022 •

edited

Loading