Add layout scan physical plan model - #9142
Conversation
joseph-isaacs
added
the
changelog/feature
ChatGPT Codex Connector
Merging this PR will degrade performance by 15.91%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
c2bc293 to
ed3b127
Compare
746ec3b to
5513849
Compare
55e7a8c to
c1b69a3
Compare
c1b69a3 to
3a71807
Compare
9914580 to
0f4ba4a
Compare
0f4ba4a to
00c5f8f
Compare
| Some(EmptyMetadata) | ||
| } | ||
|
|
||
| fn with_children( |
There was a problem hiding this comment.
I assume you want to validated each child as well here?
| } | ||
|
|
||
| /// Returns a child collection with one slot replaced. | ||
| pub fn with_child(&self, index: usize, child: PlanRef) -> VortexResult<Self> { |
There was a problem hiding this comment.
this is funky... you just keep stacking closures for any child which I think can fail very easily
There was a problem hiding this comment.
Just very slow for no reason.
| /// Creates a concatenation over `children`. | ||
| /// | ||
| /// Every child must produce `dtype`, and the row domain is the sum of the child row counts. | ||
| pub fn try_new(dtype: DType, children: Vec<PlanRef>) -> VortexResult<Self> { |
There was a problem hiding this comment.
You lose laziness here, not sure though how to think about try_new vs other constructors in this case
There was a problem hiding this comment.
There is a unchecked form_children here
| ) -> fmt::Result { | ||
| let children = plan.children(); | ||
| for index in 0..children.len() { | ||
| let child = plan.child_required(index).map_err(|_| fmt::Error)?; |
There was a problem hiding this comment.
This is unfortunate, we shouldn't swallow it but fmt error is very basic :/
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
00c5f8f to
cdea383
Compare
3 participants
see #9062
This PR add the first impl of plan nodes mirroring some layout readers.
Summary
Plan/PlanRefmodel for layout scan planning