fix(core): strip decorators while maintaining valid CSR structure #2524
Conversation
Fixes the `MastForest::strip_decorators()` method to properly strip decorator information while maintaining valid CSR (Compressed Sparse Row) structure invariants. The previous implementation called `DebugInfo::clear()` which removed the entire CSR structure. However, code paths that serialize or access decorators expect the CSR structure to exist (even if empty), causing panics when accessing decorator information after stripping. 1. Added `DebugInfo::empty_for_nodes(num_nodes)` method that creates an empty but valid CSR structure for a given number of nodes 2. Updated `strip_decorators()` to use this new method instead of `clear()` 3. Added early returns to `OpToDecoratorIds::from_components()` to support empty CSR structures (nodes with no decorators) 4. Changed `from_components` visibility from `#[cfg(test)]` to `pub(crate)` to allow production use
Added three comprehensive edge case tests: 1. test_strip_decorators_empty_forest - verifies stripping on empty forest 2. test_strip_decorators_idempotent - verifies double stripping is safe 3. test_strip_decorators_multiple_node_types - verifies CSR structure maintains validity across different node types (Block, Join, Split)
huitseeker
force-pushed
the
huitseeker/strip-decorators-properly
branch
from
47ba940 to
608cc2d
Compare
| let mut node_indptr_for_op_idx = IndexVec::new(); | ||
| for _ in 0..=num_nodes { | ||
| let _ = node_indptr_for_op_idx.push(0); | ||
| } |
There was a problem hiding this comment.
Should we just initialize the vectors with N+1 zeros, instead of pushing them one by one.
| pub fn strip_decorators(&mut self) { | ||
| // Clear all debug info (decorators and error codes) | ||
| self.debug_info.clear(); | ||
| self.debug_info = DebugInfo::empty_for_nodes(self.nodes.len()); | ||
| } |
There was a problem hiding this comment.
Maybe not for this PR, but we should probably rename this into clear_debug_info() or something like that because, AFAICT, this is also removing error messages and in the future other things as well (e.g., see #2474).
Also, I still don't fully understand why we need to preserve any info in the debug info upon clearing it. The PR description mentions some code paths that rely on this - what are these code paths? Basically, is is something we can change after #2470 is merged?
I think the ideal end state would be to have debug_info in MastForest be an optional field (i.e., debug_info: Option<DebugInfo>) - what is preventing us from doing this?
|
Am going to propose a better fix in main : #2529 |
Fixes the
MastForest::strip_decorators()method to properly stripdecorator information while maintaining valid CSR (Compressed Sparse Row)
structure invariants.
The previous implementation called
DebugInfo::clear()which removedthe entire CSR structure. However, code paths that serialize or access
decorators expect the CSR structure to exist (even if empty), causing
panics when accessing decorator information after stripping.
DebugInfo::empty_for_nodes(num_nodes)method that createsan empty but valid CSR structure for a given number of nodes
strip_decorators()to use this new method instead ofclear()OpToDecoratorIds::from_components()to supportempty CSR structures (nodes with no decorators)
from_componentsvisibility from#[cfg(test)]topub(crate)to allow production use