-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat: add DuckDB join_asof
#1860
Conversation
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.
amazing, thanks so much! just some minor comments
narwhals/_arrow/dataframe.py
Outdated
strategy: Literal["backward", "forward", "nearest"], | ||
suffix: str = "_right", |
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.
really minor, but can we avoid setting the default in the compliant level? So, in narwhals.dataframe.py
, we can have suffix: str = "_right"
, but in the internal methods in _arrow
/ _duckdb
/ _pandas_like
/ etc., we can just have suffix: str
, to make sure that we're always passing it down?
narwhals/_duckdb/dataframe.py
Outdated
strategy: Literal["backward", "forward", "nearest"] = "backward", | ||
suffix: str = "_right", | ||
) -> Self: | ||
import duckdb |
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.
i think we can do this at the top of the file
narwhals/_duckdb/dataframe.py
Outdated
msg = "Only 'backward' and 'forward' strategies are currently supported for DuckDB" | ||
raise NotImplementedError(msg) | ||
condition = " and ".join(conditions) | ||
select = [f'lhs."{x}"' for x in lhs.columns] |
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.
🤔 does `select = ['lhs.*'] work?
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 for the PR @raisadz 🙌🏼
As a tiny nitpick: since you moved the parsing logic at DataFrame level, it would be good to remove the argument default values in the compliant implementation
narwhals/dataframe.py
Outdated
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.
Nice one! 👌
tests/frame/join_test.py
Outdated
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.
Amazing work here as well 🎻
…select all left df cols with star
Thank you @MarcoGorelli and @FBruzzesi for your reviews! I addressed your comments |
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 @raisadz !
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