feat: Allow expressions as group_by keys#2325
Conversation
@FBruzzesi 😂 I'll try taking a look soon UpdateAh the issue is |
|
@FBruzzesi for narwhals/narwhals/_polars/namespace.py Lines 54 to 55 in 4d2b9d5
They essentially need something like this, but I've been putting off doing it since they have waaaaaay more methods 😔 narwhals/narwhals/_polars/dataframe.py Lines 59 to 85 in 4d2b9d5 |
group_by keysgroup_by keys
Resolves them depending on `list.copy` > Cannot access attribute "copy" for class "Sequence[str]" Attribute "copy" is unknown
This comment was marked as resolved.
This comment was marked as resolved.
- Don't use `pl.Expr` for `_keys` - We technically don't need that attribute for `polars` anyway - Use new `IncompletePolarsExpr` - Don't require `_parse_expr` for `Polars*` - It doesn't need that method - Still planning to change it later in (#2325 (comment))
Was needed earlier in the PR, but now unused
Ended up defining in `CompliantDataFrame` after
dangotbanned
left a comment
There was a problem hiding this comment.
Great work @FBruzzesi 🎉
I've left some comments/suggestions, but nothing blocking.
This is an exciting step towards expressifying more of narwhals 🚀
Thanks @dangotbanned - I am a bit short on time this week and the next - so apologies for my slow feedback loop.
It definitly is - exciting times! |
dangotbanned
left a comment
There was a problem hiding this comment.
Thanks again @FBruzzesi
I'll leave open until the end of the day in case @MarcoGorelli wants time chime in - but otherwise I'll merge later ❤️
|
thanks both, really amazing stuff! i'll just trying breaking this a bit more, then i'd say we can ship it |
@MarcoGorelli wanted to check in to see if you were okay with me merging? No worries at all if not! |
MarcoGorelli
left a comment
There was a problem hiding this comment.
I reverted a change to an existing test - in general i'd suggest to just add a new test if you're testing something different
For anything this close to the library core and to expression parsing, please do wait for me to take a final look
Anyway, this looks great, thanks @FBruzzesi and @dangotbanned ! You have a lot to be proud of here. I'm certainly proud that all together we've arrived at such a simple-looking solution
type checking does pass in CI, but there is the occasional squiggly line in vscode. happy to resolve that separately though
|
@MarcoGorelli would we be able to set up something to preserve co-authors? I think I'm missing again 😔 |
|
ah so sorry 😩 😳 the combination of 124 commit messages was too long so i removed the whole thing, but it looks like i should've kept the "co-authored by" part at the end for co-authors to show up I'll be very careful to always do that in the future 👍 |

What type of PR is this? (check all applicable)
Related issues
nw.col/Exprfor.group_by#1385Checklist
If you have comments or can explain your changes, please do so below
@MarcoGorelli there is one test failing - which is grouping by a pandas column with a boolean name (and similarly with integer name in scikit-lego)
@dangotbanned I am crying for help with the typing 🙈
Other comment will address in the code itself