-
Notifications
You must be signed in to change notification settings - Fork 0
Rework on serialization and deserialization #252
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
Conversation
core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/planner/physical/ProjectOperator.java
Outdated
Show resolved
Hide resolved
...earch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchPagedIndexScan.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]> Step 2. Signed-off-by: Yury-Fridlyand <[email protected]>
7a9d285 to
ef44600
Compare
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
This comment was marked as spam.
This comment was marked as spam.
core/src/main/java/org/opensearch/sql/planner/SerializablePlan.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/planner/SerializablePlan.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/executor/pagination/PaginatedPlanCache.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
| return new Cursor(CURSOR_PREFIX | ||
| + serialize(((SerializablePlan) plan).getPlanForSerialization())); | ||
| // ClassCastException thrown when a plan in the tree doesn't implement SerializablePlan | ||
| } catch (ClassCastException | NoCursorException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can ClassCastException be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are always trying to convert a plan tree to a cursor in
Line 55 in 10ba1d6
| plan.getTotalHits(), paginatedPlanCache.convertToCursor(plan)); |
If tree contains a plan which doesn't implement
SerializablePlan, there would be a ClassCastException somewhere, e.g. in Lines 93 to 96 in 10ba1d6
| @Override | |
| public SerializablePlan getPlanForSerialization() { | |
| return (SerializablePlan) delegate; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't serialize PaginateOperator (to reduce cursor size), so I can't check plan instanceof PaginateOperator here as it was before.
opensearch-project-sql/core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java
Lines 69 to 73 in 0ff3390
| /** No need to serialize a PaginateOperator, it actually does nothing - it is a wrapper. */ | |
| @Override | |
| public SerializablePlan getPlanForSerialization() { | |
| return (SerializablePlan) input; | |
| } |
That means I have to do another protection here. The incoming tree could be
ProjectOperator -> ResourceMonitorPlan -> PrometheusIndexScan or ProjectOperator -> ValuesOperator.
Signed-off-by: Yury-Fridlyand <[email protected]>
|
Superseded by opensearch-project#1498 |
Description
Rework
PhysicalPlantree serialization and deserialization. Make it scalable and extendable.If a new plan should be serialized (a new feature is becoming supported by pagination) - it should properly overload
SerializablePlan::writeExternalonly.The plan tree will be recovered in deserialization the same way it was before serialization.
New mechanism can also skip plans from serialization - for example, it skips
ResourceMonitorPlan.Issues Resolved
opensearch-project#1483
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.