fix: GroupBy with mixed length exprs as keys#3004
Conversation
narwhals/_compliant/group_by.py
Outdated
|
|
||
| output_names = _evaluate_aliases(compliant_frame, keys) | ||
|
|
||
| output_names_grouped = [expr._evaluate_aliases(compliant_frame) for expr in keys] |
There was a problem hiding this comment.
If someone has a better variable name I am open to suggestions. Meaning is: this is not flattened
There was a problem hiding this comment.
If someone has a better variable name I am open to suggestions
aliases_(per|by)_expr?
I'm mostly just confused about using output_names for this
narwhals/narwhals/_compliant/expr.py
Lines 154 to 158 in fe7a3e1
narwhals/_compliant/group_by.py
Outdated
| # otherwise it's single named and we can use Expr.alias | ||
| else key.alias(_temporary_name(new_name)) | ||
| for key, new_name in zip(keys, output_names) | ||
| else key.alias(_temporary_name(new_name[0])) |
There was a problem hiding this comment.
Since it's not multi output, new_name is guaranteed to have length 1
FBruzzesi
added
the
high priority
FBruzzesi
changed the title
GroupBy with mixed length exprs as keys
narwhals/_compliant/group_by.py
Outdated
| else key.alias(_temporary_name(new_name[0])) | ||
| for key, new_name in zip(keys, output_names_grouped) |
There was a problem hiding this comment.
Since they aren't flattened anymore, new_name makes it seem like we're renaming to the first character 😳
| else key.alias(_temporary_name(new_name[0])) | |
| for key, new_name in zip(keys, output_names_grouped) | |
| else key.alias(_temporary_name(new_names[0])) | |
| for key, new_names in zip(keys, output_names_grouped) |
dangotbanned
added
pyarrow
MarcoGorelli
left a comment
MarcoGorelli
left a comment
There was a problem hiding this comment.
nice, thanks @FBruzzesi for the quick fix
3 participants
What type of PR is this? (check all applicable)
Related issues
Bug spotted and reported in #3003 (comment)
Checklist
If you have comments or can explain your changes, please do so below