-
Notifications
You must be signed in to change notification settings - Fork 211
chore: Track ExpansionKind in ExprMetadata
#2266
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
Merged
Merged
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
8550da7
wip
MarcoGorelli f7f0a90
lint
MarcoGorelli 2a799c8
remove unnecessary check
MarcoGorelli 40aea92
clean up duckdb
MarcoGorelli bac71ce
clean up spark-like
MarcoGorelli c8f483d
snake case
MarcoGorelli 3fd7284
factor out `resolve_expansion_kind`
MarcoGorelli d06b4e5
completely remove function_name from duckdb and pyspark
MarcoGorelli 291bd10
remove --offline
MarcoGorelli 385fd13
remove outdated arg
MarcoGorelli 2d4dd8a
chore(typing): Ignore issues from #2263
dangotbanned a58c583
refactor: Add `DepthTrackingExpr`
dangotbanned 8e4bed2
Merge remote-tracking branch 'upstream/main' into pr/MarcoGorelli/2266
dangotbanned 274fd60
revert: Add both classmethods back
dangotbanned e333fa3
refactor: Add `DepthTrackingNamespace`
dangotbanned 5e66ae5
revert: Undo copy/paste from (https://github.com/narwhals-dev/narwhal…
dangotbanned 94cc379
ci(typing): ignore, unused ignore
dangotbanned b5b1c92
test: Kinda unbreak `sqlframe` test
dangotbanned 8fa9f1c
refactor: Hide `CompliantExpr` internals from `LazyGroupBy`
dangotbanned 3129332
Remove comment
dangotbanned 8dbeb0e
Merge remote-tracking branch 'upstream/main' into expansion-kind
MarcoGorelli ba235aa
Merge remote-tracking branch 'upstream/main' into pr/MarcoGorelli/2266
dangotbanned 3cdc967
redo (94cc379edf25f749693306ac0a126517f61bb9ad)
dangotbanned File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,8 +84,6 @@ class CompliantExpr(Protocol38[CompliantFrameT, CompliantSeriesOrNativeExprT_co] | |
| _version: Version | ||
| _evaluate_output_names: Callable[[CompliantFrameT], Sequence[str]] | ||
| _alias_output_names: Callable[[Sequence[str]], Sequence[str]] | None | ||
| _depth: int | ||
| _function_name: str | ||
| _metadata: ExprMetadata | None | ||
|
|
||
| def __call__( | ||
|
|
@@ -101,11 +99,12 @@ def from_column_names( | |
| evaluate_column_names: Callable[[CompliantFrameT], Sequence[str]], | ||
| /, | ||
| *, | ||
| function_name: str, | ||
| context: _FullContext, | ||
| ) -> Self: ... | ||
| @classmethod | ||
| def from_column_indices(cls, *column_indices: int, context: _FullContext) -> Self: ... | ||
| def from_column_indices( | ||
| cls: type[Self], *column_indices: int, context: _FullContext | ||
| ) -> Self: ... | ||
|
|
||
| def _with_metadata(self, metadata: ExprMetadata) -> Self: ... | ||
|
|
||
|
|
@@ -272,25 +271,64 @@ def __invert__(self) -> Self: ... | |
| def broadcast( | ||
| self, kind: Literal[ExprKind.AGGREGATION, ExprKind.LITERAL] | ||
| ) -> Self: ... | ||
| def _is_multi_output_agg(self) -> bool: | ||
| """Return `True` for multi-output aggregations. | ||
| def _is_multi_output_unnamed(self) -> bool: | ||
| """Return `True` for multi-output aggregations without names. | ||
|
|
||
| For example, column `'a'` only appears in the output as a grouping key: | ||
|
|
||
| Here we skip the keys, else they would appear duplicated in the output: | ||
| df.group_by('a').agg(nw.all().sum()) | ||
|
|
||
| df.group_by("a").agg(nw.all().mean()) | ||
| It does not get included in: | ||
|
|
||
| nw.all().sum(). | ||
| """ | ||
| return self._function_name.split("->", maxsplit=1)[0] in {"all", "selector"} | ||
| assert self._metadata is not None # noqa: S101 | ||
| return self._metadata.expansion_kind.is_multi_unnamed() | ||
|
|
||
|
|
||
| class DepthTrackingExpr( | ||
| CompliantExpr[CompliantFrameT, CompliantSeriesOrNativeExprT_co], | ||
| Protocol38[CompliantFrameT, CompliantSeriesOrNativeExprT_co], | ||
| ): | ||
| _depth: int | ||
| _function_name: str | ||
|
|
||
| @classmethod | ||
| def from_column_names( | ||
| cls: type[Self], | ||
| evaluate_column_names: Callable[[CompliantFrameT], Sequence[str]], | ||
| /, | ||
| *, | ||
| context: _FullContext, | ||
| function_name: str = "", | ||
| ) -> Self: ... | ||
|
|
||
| def _is_elementary(self) -> bool: | ||
| """Check if expr is elementary. | ||
|
|
||
| Examples: | ||
| - nw.col('a').mean() # depth 1 | ||
| - nw.mean('a') # depth 1 | ||
| - nw.len() # depth 0 | ||
|
|
||
| as opposed to, say | ||
|
|
||
| - nw.col('a').filter(nw.col('b')>nw.col('c')).max() | ||
|
|
||
| Elementary expressions are the only ones supported properly in | ||
| pandas, PyArrow, and Dask. | ||
|
Comment on lines
+318
to
+319
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied directly from Maybe a description in the class doc could have some stuff from https://narwhals-dev.github.io/narwhals/how_it_works |
||
| """ | ||
| return self._depth < 2 | ||
|
|
||
| def __repr__(self) -> str: # pragma: no cover | ||
| return f"{type(self).__name__}(depth={self._depth}, function_name={self._function_name})" | ||
|
|
||
|
|
||
| class EagerExpr( | ||
| CompliantExpr[EagerDataFrameT, EagerSeriesT], | ||
| DepthTrackingExpr[EagerDataFrameT, EagerSeriesT], | ||
| Protocol38[EagerDataFrameT, EagerSeriesT], | ||
| ): | ||
| _call: Callable[[EagerDataFrameT], Sequence[EagerSeriesT]] | ||
| _depth: int | ||
| _function_name: str | ||
| _evaluate_output_names: Any | ||
| _alias_output_names: Any | ||
| _call_kwargs: dict[str, Any] | ||
|
|
||
| def __init__( | ||
|
|
@@ -310,9 +348,6 @@ def __init__( | |
| def __call__(self, df: EagerDataFrameT) -> Sequence[EagerSeriesT]: | ||
| return self._call(df) | ||
|
|
||
| def __repr__(self) -> str: # pragma: no cover | ||
| return f"{type(self).__name__}(depth={self._depth}, function_name={self._function_name})" | ||
|
|
||
| def __narwhals_namespace__( | ||
| self, | ||
| ) -> EagerNamespace[EagerDataFrameT, EagerSeriesT, Self]: ... | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(#2266 (comment)) can be taken a step further by adding:
So this part wouldn't need to even know what an
ExpansionKindis, and having the line here be:But this works as well