Added design for core refactor #331
Conversation
Signed-off-by: Guian Gumpac <[email protected]>
GumpacG
requested review from
MaxKsyunz,
Yury-Fridlyand,
forestmvey and
acarbonetto
as code owners
Codecov Report
@@ Coverage Diff @@
## integ-core-refactor-docs #331 +/- ##
===========================================================
Coverage 97.42% 97.42%
Complexity 4647 4647
===========================================================
Files 408 408
Lines 11526 11526
Branches 839 839
===========================================================
Hits 11229 11229
Misses 290 290
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| ``` | ||
|
|
||
| ```mermaid | ||
| flowchart LR |
There was a problem hiding this comment.
Which is the preferred diagram here? Sequence or flowchart?
| ``` | ||
|
|
||
| ```mermaid | ||
| flowchart LR |
|
|
||
| # 2. Requirements | ||
|
|
||
| Resolve https://github.com/opensearch-project/sql/issues/811 |
There was a problem hiding this comment.
Please, add more info. Probably, describe a user story with this feature implemented: plugin for plugin case. It also add dependency clean up and extensibility.
There was a problem hiding this comment.
Please, rename, core refactor is too common.
| @@ -0,0 +1,71 @@ | |||
| # 1. Overview | |||
There was a problem hiding this comment.
Add an abstract, title or intro.
| Core LogicalPlanOptimizer ->> #8249;DataSource#8250; LogicalPlanOptimizer: getList() | ||
| #8249;DataSource#8250; LogicalPlanOptimizer -->> Core LogicalPlanOptimizer: Datasource specific rules list | ||
| Core LogicalPlanOptimizer -->> Planner: LogicalPlan | ||
| ``` |
There was a problem hiding this comment.
Describe:
- discovered DS interfaces, for example:
parsefor UDFparsefor identifier- analyze, optimize, etc
- API to plug/attach new DS on runtime
- how parser with UDF should work
- algorithm of resolution of a DS for a query with examples; consider tricky examples, when UDF or identifier exists in multiple DS
- override policy and workflow (in case if a DS overrides a built-in function), or if it is prohibited - how it is checked
- Query processing flow and response if UDF/identifier isn't resolved
|
|
||
| # 2. Requirements | ||
|
|
||
| Resolve https://github.com/opensearch-project/sql/issues/811 |
There was a problem hiding this comment.
Does this resolve the entire issue? According to opensearch-project#811 (comment), I believe this only resolves core functions. But does it also refactor the parser and pull out the push-down functions?
There was a problem hiding this comment.
In other words, I think we need to list exactly what we are resolving as part of issue opensearch-project#811.
| @@ -0,0 +1,71 @@ | |||
| # 1. Overview | |||
There was a problem hiding this comment.
Can we name this file a bit more specifically? I think core-refactor doesn't describe the changes or objectives accurately enough.
|
|
||
| - Using data sources other than OpenSearch, users will encounter a `SyntaxCheckException` when they query with OpenSearch specific functions. (Some of these functions can be found in https://opensearch.org/docs/latest/search-plugins/sql/sql/functions/) | ||
| - The Parser will have a different token to categorize OpenSearch functions. | ||
| - Core module will no longer contain OpenSearch functions. |
There was a problem hiding this comment.
This is the #1 goal, right?
Everything is a by-product of getting the functions out of the core module.
| ## 2.2 Functional change | ||
|
|
||
| - Using data sources other than OpenSearch, users will encounter a `SyntaxCheckException` when they query with OpenSearch specific functions. (Some of these functions can be found in https://opensearch.org/docs/latest/search-plugins/sql/sql/functions/) | ||
| - The Parser will have a different token to categorize OpenSearch functions. |
There was a problem hiding this comment.
This is a architectural design element, not functional. The users won't see this, nor is this a requirement.
Description
Proposal for refactoring core to remove OpenSearch specific functions.
Issues Resolved
opensearch-project#811
Check List
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.