implement transport api for PPL inter-plugin communication

[zhongnansu]

Signed-off-by: Zhongnan Su [email protected]

Update

After discussion in the thread here opensearch-project/common-utils#155, we decided for now we'll keep the transport interfaces in sql repo, instead of common-utils for the following reasons.

1. there's no consumer plugin for this feature yet. Because the response format is not encapsulated. Currently on trasnport layer there's only a json string in the response. Ideally we want a ResultSet or DataFrame like response for clients to access data. An issued is created w.r.t this.
2. This change is to the request model, which is the input of the SQL/PPL engine. Having it separated into 2 repos is not development friendly. E.g, the CI will fail for sure, because CI only pulls against released common-utils

Description

Add a transport layer for handle PPL request. This will expose transport interface for inter-plugin communication. The existing Rest layer handler is also being refactored to make use of the transport layer

• Create TransportPPLQueryAction

• refactored RestPPLQueryAction to service as a route layer, moved the query request handling logic to TransportPPLQueryAction.

• Register action and handler in SQLPlugin

• Refactored PPLQueryRequest to be writable, for the transport layer commuication

• Create transport response model that currently holds a string response.

• Add common-utils as dependency, since the request/response models will be exposed in common-utils for other plugin to consume as well.

• TODO:

  1. Request and response format are strings for now.(may need modification in future version, especially response format)
  2. Implement transport interface for SQL, in a similar manner

Issues Resolved

#521

CI/CD

• The CI is failing because of the pending PR in common-utils
• [Fixed]local build failed because of jarhell caused by ml-commons. Raised another PR in their repo to fix. Fix jarhell of common-utils ml-commons#328

| java.home: /Library/Java/JavaVirtualMachines/jdk-14.0.2.jdk/Contents/Home
| examining directory: /Users/szhongna/Desktop/Projects/OpenSearch/sql/integ-test/build/classes/java/test
| examining directory: /Users/szhongna/Desktop/Projects/OpenSearch/sql/integ-test/build/resources/test
| examining jar: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar
| examining jar: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-1.2.0.0.jar
| Exception in thread "main" java.lang.IllegalStateException: jar hell!
| class: org.opensearch.jdbc.ResultSetImpl
| jar1: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar
| jar2: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-1.2.0.0.jar
|       at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:317)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:212)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:99)
|       at org.opensearch.bootstrap.JarHell.main(JarHell.java:83)

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':integ-test:jarHell'.
> Process 'command '/Library/Java/JavaVirtualMachines/jdk-14.0.2.jdk/Contents/Home/bin/java'' finished with non-zero exit value 1

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.

[codecov-commenter]

Codecov Report

Merging #533 (8bc8299) into main (14fc606) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #533      +/-   ##
============================================
+ Coverage     94.59%   94.67%   +0.08%     
- Complexity     2743     2801      +58     
============================================
  Files           276      280       +4     
  Lines          7455     7570     +115     
  Branches        552      558       +6     
============================================
+ Hits           7052     7167     +115     
  Misses          349      349              
  Partials         54       54              

Flag	Coverage Δ
query-workbench	62.76% <ø> (ø)
sql-engine	97.71% <100.00%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
...org/opensearch/sql/ppl/domain/PPLQueryRequest.java	100.00% <100.00%> (ø)
...c/main/java/org/opensearch/sql/expression/DSL.java	100.00% <0.00%> (ø)
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	100.00% <0.00%> (ø)
...pensearch/sql/ppl/parser/AstExpressionBuilder.java	100.00% <0.00%> (ø)
...pensearch/sql/sql/parser/AstExpressionBuilder.java	100.00% <0.00%> (ø)
...ensearch/sql/expression/ExpressionNodeVisitor.java	100.00% <0.00%> (ø)
...h/sql/expression/function/BuiltinFunctionName.java	100.00% <0.00%> (ø)
...h/sql/expression/function/OpenSearchFunctions.java	100.00% <0.00%> (ø)
...arch/storage/script/filter/FilterQueryBuilder.java	100.00% <0.00%> (ø)
... and 5 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 14fc606...8bc8299. Read the comment docs.

[penghuo]

seems missing files, SQLActions, TransportQueryResponse

[zhongnansu]

seems missing files, SQLActions, TransportQueryResponse

@penghuo Those are interfaces that should be defined in common-utils and used by any caller plugins. Currently I have a PR pending for common-utils. Once that's merged and released to maven central, this PR will pass CI

[penghuo]

Two things.

1. RestSQLAction should be refactored to call TransportSQLQueryAction.
2. Is it possible to add UT?

[zhongnansu]

Two things.

1. RestSQLAction should be refactored to call TransportSQLQueryAction.
2. Is it possible to add UT?

Just reply to all other comments in one thread. Thanks a lot for the advice. @penghuo

1. I merged the TransportPPLService into transportPPLQueryAction, refactored RestPPLQueryAction to service as a route layer, moved the query request handling logic to TransportPPLQueryAction.
2. Raise another PR Fix jarhell of common-utils ml-commons#328 to address the jarhell issue
3. Since it will affect lots of usage in the code, I added some TODOs comments to move the PPLQueryRequest data model into common-utils. Currently we are converting the PPLQueryRequest and TransportPPLQueryRequest(common-utils) manually.
4. Update PR in common-utils to refactor TrasnportPPLQueryRequest model, also moved Style enum from sql to common-utils since it's also used in PPLQueryRequest and SQLQueryRequest. add SQL/PPL transport request/response models common-utils#155

[zhongnansu]

Addressed all comments. Local build keeps failing during ./gradlew build, > Task :integ-test:jarHell FAILED

| sun.boot.class.path: null
| java.home: /Library/Java/JavaVirtualMachines/jdk-14.0.2.jdk/Contents/Home
| examining directory: /Users/szhongna/Desktop/Projects/OpenSearch/sql/integ-test/build/classes/java/test
| examining directory: /Users/szhongna/Desktop/Projects/OpenSearch/sql/integ-test/build/resources/test
| examining jar: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar
| examining jar: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-1.2.0.0.jar
| Exception in thread "main" java.lang.IllegalStateException: jar hell!
| class: org.opensearch.jdbc.ResultSetImpl
| jar1: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar
| jar2: /Users/szhongna/Desktop/Projects/OpenSearch/sql/sql-jdbc/build/libs/opensearch-sql-jdbc-1.2.0.0.jar
|       at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:317)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:212)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:99)
|       at org.opensearch.bootstrap.JarHell.main(JarHell.java:83)

It doesn't related to my change. Any idea how can I resolve this? @penghuo @joshuali925

[zhongnansu]

doctest and gradle test gradle integTest can pass

[penghuo]

build/libs/opensearch-sql-jdbc-1.2.0.0.jar

clean you local build folder will help.

[zhongnansu]

@penghuo Hi Peng, I have moved the req/response model from common-utils to sql repo, and added unit test. CI now can pass. Could you please take a look? Thanks

[penghuo]

Thanks for the change!