-
Notifications
You must be signed in to change notification settings - Fork 147
add coalesce for all backends #2647
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
base: main
Are you sure you want to change the base?
Conversation
I'll probably need to write more tests, at the very least this should serve as a functional MVP. |
Hold off on merging for now, I need some tests for coalescing across disparate datatypes. |
@MarcoGorelli thoughts on the following considerations for
|
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.
thanks @camriddell !
i think what you've done is fine and we shouldn't do any casting on behalf of the user, let the backends handle that as they wish
narwhals/functions.py
Outdated
flat_exprs = flatten( | ||
[ | ||
expr if isinstance(expr, (Expr, str)) else lit(expr) | ||
for expr in chain(flatten([exprs]), more_exprs) | ||
] | ||
) |
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.
this looks a bit suspicious, can we reuse anything? check concat_str
or any of the other functions in this file
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.
The main idea was to coerce any inputs that are not strings/expressions into literals. Though this would also mean that any valid nw.Series
objects would be inappropriately coerced as well.
Is there a helper somewhere that does this? If not, I've made the change to remove this capability- reverting is easy.
narwhals/_duckdb/namespace.py
Outdated
def coalesce(self, *exprs: DuckDBExpr) -> DuckDBExpr: | ||
def func(df: DuckDBLazyFrame) -> list[Expression]: | ||
cols = (c for _expr in exprs for c in _expr(df)) | ||
return [CoalesceOperator(*cols)] | ||
|
||
return self._expr( | ||
call=func, | ||
evaluate_output_names=combine_evaluate_output_names(*exprs), | ||
alias_output_names=combine_alias_output_names(*exprs), | ||
backend_version=self._backend_version, | ||
version=self._version, | ||
) |
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.
this should be elementwise, right? can we use the new _with_elementwise
here for duckdb and pyspark?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below
Adds a
narwhals.coalesce
function for use with Narwhals Expressions.