Skip to content
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

Rationalize type of (key, val) to row mapping #27027

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

frankmcsherry
Copy link
Contributor

@frankmcsherry frankmcsherry commented May 10, 2024

We use a BTreeMap<usize, usize> for the "permutation" from concatenated [key, val] columns, when it appears it can always be a Vec<usize>: each output column must identify an input column. This also hints that this isn't really a permutation as much as a projection. More generally both of these could be Vec<MirScalarExpr> if we ever plan to be so bold (and perhaps this is an opportunity to abstract the specifics away, so that everyone's signatures don't change in the future). For example, various permute functions could/should be pre-mfp where you hand over perhaps a Vec<MirScalarExpr> that could be column references, or more general expressions. E.g. if we form a key using a cast, we cannot represent the operation that just un-casts it (in some cases) and that would be good enough vs keeping the un-cast column around as a value.

Motivation

Tips for reviewer

Checklist

Comment on lines +290 to +293
safe_mfp.permute(
permute.into_iter().enumerate().collect(),
key.len() + thinning.len(),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change permute to accept a &[usize] instead? If I'm reading this correctly, all uses are of the form permute(permute.into_iter().enumerate().collect(), ...), unless there are some outside of this change that I can't see here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants