feat(expr): support any children in rw_vnode
#18405
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @BugenZhao and the rest of your teammates on |
| for child in &self.children { | ||
| arrays.push(child.eval(input).await?); | ||
| } | ||
| let input = DataChunk::new(arrays, input.visibility().clone()); |
There was a problem hiding this comment.
A little overhead of allocation.
|
Do we really need to "debug" with this? |
|
|
||
| Ok(Box::new(VnodeExpression { dist_key_indices })) | ||
| Ok(Box::new(VnodeExpression { | ||
| all_indices: (0..children.len()).collect(), |
There was a problem hiding this comment.
I am confused by this field, although it's renamed from dist_key_indices. Can you please exemplify and explain its usage?
There was a problem hiding this comment.
It's a parameter required by VirtualNode::compute_row to project the distribution key columns. But it's true that this can be done in the callsite by simply attaching .project. Will change the signature.
There was a problem hiding this comment.
Oh, it's also used by VirtualNode::compute_chunk.
There was a problem hiding this comment.
input.project(dist_key_indices) is changed to children.eval(input) so no dist_key_indices needed anymore.
all_indices is a new field introduced to reduce redundant vec allocation.
There was a problem hiding this comment.
Maybe it's better to rename children to dist_key_exprs or sth.
BugenZhao
force-pushed
the
bz/vnode-expr-any-input
branch
from
7889c32 to
68ff5ea
Compare
Signed-off-by: Bugen Zhao <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Previously, if the children of
rw_vnodeis notInputRef, it will panic. This is okay as most of the time, it's only used internally for 2-phase aggregation. However, since it's still user-facing for debugging purposes, we should make it more user-friendly.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.