Skip to content

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jun 21, 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

Having the method defined on the Namespace (current), is what I was trying to avoid since

Prior to that, we had things like:

  • Namespace._create_series_from_scalar
  • Namespace._create_expr_from_callable
  • Namespace._create_expr_from_series

I'd think it's much simpler to keep constructors defined on the class they produce, and they also benefit from shorter names πŸ™‚

Comment on lines 897 to 900
@classmethod
def from_elementwise(
cls, func: Callable[[Iterable[NativeExprT]], NativeExprT], *exprs: Self
) -> Self: ...
Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoGorelli would you be open to adding a little doc here? πŸ™

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, another thought.

What about LazyExpr.from_horizontal_op?

That would mirror the same constructor these all use for ExprMetadata

narwhals/narwhals/functions.py

Lines 1369 to 1374 in f17acfc

return Expr(
lambda plx: apply_n_ary_operation(
plx, plx.sum_horizontal, *flat_exprs, str_as_lit=False
),
ExprMetadata.from_horizontal_op(*flat_exprs),
)

@classmethod
def from_horizontal_op(cls, *exprs: IntoExpr) -> ExprMetadata:
return combine_metadata(
*exprs, str_as_lit=False, allow_multi_output=True, to_single_output=True
)

I think the detail that we should use this for *_horizontal (and concat_str) is more relevant than the elementwise part

cls, func: Callable[[Iterable[Expression]], Expression], *exprs: Self
) -> Self:
def call(df: DuckDBLazyFrame) -> list[Expression]:
cols = (col for _expr in exprs for col in _expr(df))
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 think these could just be:

return [func(chain.from_iterable(expr(df) for expr in exprs))]

@dangotbanned dangotbanned marked this pull request as ready for review June 22, 2025 13:42
@MarcoGorelli MarcoGorelli changed the title refactor: Enforce LazyExpr.from_elementwise refactor: Enforce LazyExpr._from_horizontal_op Jun 27, 2025
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.

thanks @dangotbanned

i've renamed and prefixed with an underscore as I don't think this should be part of the compliant protocols #2713, but fine for now

@dangotbanned
Copy link
Member Author

thanks @dangotbanned

i've renamed and prefixed with an underscore as I don't think this should be part of the compliant protocols #2713, but fine for now

Thanks @MarcoGorelli!

One thing though - the name in the PR title doesn't match the code πŸ˜‰

I prefer the one without elementwise

@MarcoGorelli MarcoGorelli changed the title refactor: Enforce LazyExpr._from_horizontal_op refactor: Enforce LazyExpr._from_elementwise_horizontal_op Jun 27, 2025
@MarcoGorelli
Copy link
Member

thanks! same, but it does need to be elementwise if we use window_function the way it's defined inside it

@MarcoGorelli MarcoGorelli merged commit 24519da into main Jun 27, 2025
33 checks passed
@MarcoGorelli MarcoGorelli deleted the lazy-expr-from-elementwise branch June 27, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants