Skip to content
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

add SQL/PPL transport request/response models #155

Closed

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Apr 4, 2022

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

Description

  • Add the transport interfaces for SQL plugin, to enable sending PPL/SQL query between other plugin in transport calls
  • Define corresponding request/response data models that any plugin can consume
  • Related PR in SQL plugin won't pass CI, until this PR is merged and artifacts is released to central repo.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc 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.

@zhongnansu zhongnansu marked this pull request as ready for review April 5, 2022 03:01
@zhongnansu zhongnansu requested review from a team, anirudha and joshuali925 April 5, 2022 03:01
@dblock
Copy link
Member

dblock commented May 5, 2022

@zhongnansu This looks old and unattended, apologies. Do you still want this merged? I'll find someone to CR.

@getsaurabh02
Copy link
Member

@zhongnansu Are you still blocked on review for this PR?

@zhongnansu
Copy link
Member Author

@dblock @getsaurabh02 Yes, I need to merge this in. At that time I don't want it to affect 2.0 release. Now since 2.0 is cut, I can contribute this feature. will update the PR today and re-request review, thx

joshuali925
joshuali925 previously approved these changes May 10, 2022
* Action Response for ppl and sql query.
*/
class TransportQueryResponse : BaseResponse {
val queryResponse: String
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of return plain string, we talked about DataFrame/DataSet. Please create an issue to describe the idea and link to this PR.

@zhongnansu zhongnansu force-pushed the sql-transport-api branch from 9f99617 to 005cf4d Compare May 24, 2022 09:59
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #155 (005cf4d) into main (a03453c) will decrease coverage by 0.15%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##               main     #155      +/-   ##
============================================
- Coverage     83.76%   83.60%   -0.16%     
- Complexity      434      472      +38     
============================================
  Files            66       74       +8     
  Lines          2205     2306     +101     
  Branches        254      257       +3     
============================================
+ Hits           1847     1928      +81     
- Misses          261      280      +19     
- Partials         97       98       +1     
Impacted Files Coverage Δ
...n/org/opensearch/commons/utils/TransportHelpers.kt 44.44% <ø> (-55.56%) ⬇️
...ch/commons/sql/action/TransportPPLQueryResponse.kt 63.63% <63.63%> (ø)
.../org/opensearch/commons/sql/action/BaseResponse.kt 66.66% <66.66%> (ø)
...rch/commons/sql/action/TransportPPLQueryRequest.kt 83.78% <83.78%> (ø)
...n/org/opensearch/commons/sql/SQLPluginInterface.kt 86.66% <86.66%> (ø)
...ch/commons/sql/action/TransportSQLQueryResponse.kt 87.50% <87.50%> (ø)
...rch/commons/sql/action/TransportSQLQueryRequest.kt 94.11% <94.11%> (ø)
...in/org/opensearch/commons/sql/action/SQLActions.kt 100.00% <100.00%> (ø)
...n/kotlin/org/opensearch/commons/sql/model/Style.kt 100.00% <100.00%> (ø)

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 a03453c...005cf4d. Read the comment docs.

@penghuo
Copy link

penghuo commented May 26, 2022

I have concern regarding to maintenance the transport layer interface. Ideally, SQL/PPL should be a module of Opensearch, then other application plugins could directly use it. Could we consider move Transport interface to SQL/PPL for now. Then we can decide later how we want to separate them when we have real use case?

@praveensameneni
Copy link
Member

Ideally, SQL/PPL should be a module of Opensearch, then other application plugins could directly use it

I like this idea. @penghuo , can you create an issue on core and let's discuss if we can move this to modules so other plugins can directly use it. What do you think could be the changes needed for other plugins to use SQL/PPL without much modifications.

@zhongnansu
Copy link
Member Author

I have concern regarding to maintenance the transport layer interface. Ideally, SQL/PPL should be a module of Opensearch, then other application plugins could directly use it. Could we consider move Transport interface to SQL/PPL for now. Then we can decide later how we want to separate them when we have real use case?

@penghuo @praveensameneni Thanks for the feedback. Separating SQL/PPL codebase into 2 repos without doesn't seem like a long term maintainable solution. Besides, like your previous comment mentioned, without a response wrapper, the client will find it difficult to use just pure json. I'll close this PR and link to your new issue

@zhongnansu zhongnansu closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants