Skip to content

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Mar 19, 2025

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

- Only spotted while trying out `CompliantGroupBy`
- `CompliantLazyFrame` already has these attributes
@dangotbanned dangotbanned marked this pull request as ready for review March 19, 2025 19:27
Comment on lines +53 to +55
_implementation: Implementation
_backend_version: tuple[int, ...]
_version: Version
Copy link
Member Author

Choose a reason for hiding this comment

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

I was pretty surprised that I didn't need to change anything outside of the protocol

Looks like everyone was already compliant 😄

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

@MarcoGorelli MarcoGorelli merged commit 48ca7b7 into main Mar 20, 2025
27 of 28 checks passed
@MarcoGorelli MarcoGorelli deleted the compliant-df-context branch March 20, 2025 11:43
MarcoGorelli pushed a commit that referenced this pull request Mar 20, 2025
* feat(DRAFT): Add `CompliantGroupBy`

Based on (#2184 (comment))

* fix: `3.8` compat

Needed for `__init__` support

* chore: export to `_compliant`

* refactor: Implement `ArrowGroupBy`

* fix(typing): Get an agreement on variance

- Dropped `native` as it got too complex
- I've done this fake `Any` thing to make `mypy` understand in multiple places

* refactor: Implement `PandasLikeGroupBy`?

- Having a hard time working out what is going on here
- All I've changed is what the refs are named

* refactor: Implement `DaskLazyGroupBy`

Much happier with this than the `pandas` one

* chore(typing): Utilize (#2251)

Fixes https://github.com/narwhals-dev/narwhals/actions/runs/13968615945/job/39104706209?pr=2252

* refactor: `CompliantGroupBy._remap_expr_name`

- Shorted in each backend
- Added docs
- Accounts for `dask` deviation from `str`

* refactor: `CompliantGroupBy._leaf_name`

- More performant to compile a single pattern and reuse everywhere
- Gives a name to a common op
- Backend code is shorter

* refactor: `CompliantGroupBy._is_simple`

Keeps this part of `pandas` in sync, despite it doing some extra name stuff

* fix: pre `3.13` protocol support

https://results.pre-commit.ci/run/github/760058710/1742478308._GAah8QXT2i9H9TbtcMHwQ

* refactor: Move most of `CompliantGroupBy` -> `DepthTrackingGroupBy`

- `_duckdb` and `_spark_like` don't need these parts (only `_dask` does)
- Also avoids needing a `TypeVar` default, which caused some issues in https://github.com/narwhals-dev/narwhals/actions/runs/13970848097/job/39111959826?pr=2252

* refactor(DRAFT): Start simplifying, aligning lazy group bys

- These two are almost identical
- Trying to reduce them as much as possible, before moving the common parts to `nw._compliant.LazyGroupBy`

* help `mypy`

https://github.com/narwhals-dev/narwhals/actions/runs/13972033263/job/39116058196?pr=2252

* refactor: Adds `LazyGroupBy`

- Greatly simplifies what each backend needs to implement
- Avoids creating and combining intermediate lists
- Avoids performing a `not in exclude` check, where `exclude` is empty
- Identified and documented a new common method `CompliantExpr._is_multi_output_agg`

* feat(typing): `Polars*GroupBy` and `Compliant*Frame`

- Will need to make (inavriant) `CompliantExprT` to type the second part correctly
- Believe that is also causing this weird error

```
Argument of type "CompliantExprT_contra@CompliantDataFrame" cannot be assigned to
parameter "exprs" of type "CompliantExprT_contra@CompliantDataFrame" in function "select"
  Type
  "CompliantExprT_contra@CompliantDataFrame" is not assignable to type
  "CompliantExprT_contra@CompliantDataFrame"

Pylance(reportArgumentType)
```

* chore: Rename pattern, remove temp doc

* Long variable names < types & docs

* refactor: listcomp < genexpr

* chore(typing): Temp ignore for false positive

* avoid unused import

* refactor: Use `Implementation.is_pandas`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants