feat!: disallow concat(..., how="horizontal") for LazyFrame#2341
feat!: disallow concat(..., how="horizontal") for LazyFrame#2341MarcoGorelli merged 10 commits intonarwhals-dev:mainfrom
concat(..., how="horizontal") for LazyFrame#2341Conversation
narwhals/stable/v1/__init__.py
Outdated
| items: Iterable[LazyFrame[IntoFrameT]], | ||
| *, | ||
| how: Literal["horizontal", "vertical", "diagonal"] = "vertical", | ||
| how: Literal["vertical", "diagonal"] = "vertical", |
There was a problem hiding this comment.
Did you mean to change the signature only in v1?
There was a problem hiding this comment.
nope, thanks 😄 will fix later
|
@MarcoGorelli do we want a signature like this? And maybe one or more typing tests to validate we get a new warning from |
There was a problem hiding this comment.
Thanks @MarcoGorelli, it looks good 🥦
It might be worth updating Perfect backwards compatibility policy#breaking changes section to mention that horizontal concatenation is no longer supported for lazy frame
dangotbanned
changed the title
concat(..., how="horizontal") for LazyFrame
tests/series_only/hist_test.py
Outdated
| pytest.skip( | ||
| reason="https://github.com/narwhals-dev/narwhals/issues/2348", allow_module_level=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
@MarcoGorelli if this cleans up CI (and you're not planning to merge this PR today) could you split the fix into a tiny PR please? 🙏
There was a problem hiding this comment.
I'm always worried I'll miss something when there's existing CI issues
There was a problem hiding this comment.
Ah maybe an xfail will save us from https://github.com/narwhals-dev/narwhals/actions/runs/14376119881/job/40308843895?pr=2341
There was a problem hiding this comment.
Ah maybe an
xfailwill save us from narwhals-dev/narwhals/actions/runs/14376119881/job/40308843895?pr=2341
It did 😁 (ee52813)
| xfail_hist = pytest.mark.xfail( | ||
| reason="https://github.com/narwhals-dev/narwhals/issues/2348", strict=False |
There was a problem hiding this comment.
- I used this on every test to mimic what
skipwas doing - No idea which one(s) was causing the failures
dangotbanned
left a comment
dangotbanned
left a comment
There was a problem hiding this comment.
Good call on this one @MarcoGorelli
Thanks for working on it
| if how == "horizontal": | ||
| all_column_names: list[str] = [ | ||
| column for frame in dfs for column in frame.columns | ||
| ] | ||
| if len(all_column_names) != len(set(all_column_names)): # pragma: no cover | ||
| duplicates = [ | ||
| i for i in all_column_names if all_column_names.count(i) > 1 | ||
| ] | ||
| msg = ( | ||
| f"Columns with name(s): {', '.join(duplicates)} " | ||
| "have more than one occurrence" | ||
| ) | ||
| raise AssertionError(msg) | ||
| return DaskLazyFrame( | ||
| dd.concat(dfs, axis=1, join="outer"), | ||
| backend_version=self._backend_version, | ||
| version=self._version, | ||
| ) |
|
thanks all for reviews! i'm a little short on time, will ship this as-is then and we can improve the overloads separately |
sorry, forgot to reply - I didn't add it there because we're not preserving this behaviour in v1, as it was buggy to begin with (Dask and Polars didn't do the same thing) |
3 participants
closes #2340
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