refactor(optimizer): use Cell instead of RefCell for interior mutability
#19883
Conversation
Signed-off-by: Richard Chien <[email protected]>
…ilar to correct the semantics Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
stdrc
changed the title
Cell instead of RefCell for Cell instead of RefCell for interior mutability
| /// This should only be called in [`crate::optimizer::plan_node::reorganize_elements_id`]. | ||
| pub(in crate::optimizer) fn reset_elem_ids(&self) { | ||
| self.last_plan_node_id.set(0); | ||
| self.last_correlated_id.set(0); | ||
| self.last_expr_display_id.set(0); | ||
| } | ||
|
|
||
| pub fn next_correlated_id(&self) -> CorrelatedId { | ||
| *self.next_correlated_id.borrow_mut() += 1; | ||
| *self.next_correlated_id.borrow() | ||
| pub(in crate::optimizer) fn restore_elem_ids(&self, backup: LastAssignedIds) { | ||
| self.last_plan_node_id.set(backup.last_plan_node_id); | ||
| self.last_correlated_id.set(backup.last_correlated_id); | ||
| self.last_expr_display_id.set(backup.last_expr_display_id); |
There was a problem hiding this comment.
What about just inlining these functions info reorganize_elements_id?
There was a problem hiding this comment.
When adding a new id later, we can hardly remember that there's a reorganize_elements_id far away need to be updated. So I decided to hide all the ids inside LastAssignedIds.
There was a problem hiding this comment.
I don't get you. We can still add an id in OptimizerContext but not in LastAssignedIds
There was a problem hiding this comment.
Maybe you should replace the fields to LastAssignedIds in OptimizerContext
There was a problem hiding this comment.
The fact is that we missed last_correlated_id in reorganize_elements_id before. I want to reduce the possibility of future error, so I moved them to LastAssignedIds and put it right next to OptimizerContext. I also definitely considered directly using LastAssignedIds as a field in OptimizerContext, but IMO it's too annoying to access these ID fields via a long self.last_assigned_ids.last_plan_node_id. After all forgetting to add new id field to LastAssignedIds is not a critical issue, we just need to prevent it to a proper degree.
This PR doesn't aim to completely refactor the optimizer element IDs, otherwise I would extract an IdAllocator struct to allocate IDs instead of self.last_plan_node_id.update in OptimizerContext. Also I don't think use interior mutability here in OptimizerContext is a good idea. But I don't want to do that in this PR. It's just a quick fix to unblock my future work on other things.
…ability (#19883) Signed-off-by: Richard Chien <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| @@ -255,12 +268,12 @@ impl std::fmt::Debug for OptimizerContext { | |||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | |||
| write!( | |||
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
In
OptimizerContext, we have some fields representing the last assigned optimizer element IDs, e.g. plan node ID. Previously we useRefCellfor these fields, but what we actually want is just interior mutability, not runtime borrow checking. This PR changes these fields to useCellinstead.Checklist
Documentation
Release note