Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LazyFrame_join_asof + DataFrame_join_asof #172

Merged
merged 36 commits into from
May 4, 2023
Merged

LazyFrame_join_asof + DataFrame_join_asof #172

merged 36 commits into from
May 4, 2023

Conversation

sorhawell
Copy link
Collaborator

resolve #151

@sorhawell
Copy link
Collaborator Author

currently misses some more tests + DataFrame_join_asof

@sorhawell sorhawell marked this pull request as ready for review May 1, 2023 12:56
@sorhawell sorhawell changed the title WIP: add LazyFrame_join_asof + DataFrame_join_asof LazyFrame_join_asof + DataFrame_join_asof May 1, 2023
@eitsupi
Copy link
Collaborator

eitsupi commented May 1, 2023

Could you resolve the conflicts?

@sorhawell
merge in main
3723058 
Merge branch 'main' into join_asof

# Conflicts:
#	src/rust/src/lazy/dataframe.rs
@sorhawell sorhawell marked this pull request as draft May 2, 2023 07:24
@sorhawell sorhawell marked this pull request as ready for review May 2, 2023 13:27
@sorhawell
Copy link
Collaborator Author

@etiennebacher will you be my reviewer again? :)

@etiennebacher
Copy link
Collaborator

etiennebacher commented May 2, 2023 •
edited
Loading

Sure, I don't have time for a proper review now, but I can probably do it in the next day or two.

Just one thing I noticed is that the docs for join_asof is the same for DataFrame and LazyFrame. You could write them only for one of the two and use the roxygen tag @inheritParams in the other one to automatically duplicate the docs. That way you also ensure that future changes will be synchronized

@sorhawell
Copy link
Collaborator Author

@etiennebacher I added inherits Param. I will start using that a lot more. Thx

etiennebacher
etiennebacher requested changes May 3, 2023
View reviewed changes
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Looks good to me for the R part, I can't tell for the Rust part but I guess "if it compiles it should be fine" TM 😄

R/dataframe__frame.R Outdated Show resolved Hide resolved
R/dataframe__frame.R Outdated Show resolved Hide resolved
R/dataframe__frame.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
tests/testthat/test-dataframe.R Outdated Show resolved Hide resolved
tests/testthat/test-dataframe.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
sorhawell and others added 8 commits May 3, 2023 14:09
@sorhawell @etiennebacher
adopt suggestion sufix
1125fb8 
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@sorhawell @etiennebacher
adopt suggestion docs wording
633480d 
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@sorhawell @etiennebacher
adopt suggestion force_parallel docs
b0f9cdd 
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@sorhawell @etiennebacher
adopt docs within groups
aed096f 
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@sorhawell
reintroduce NA test
f4bca0a
@sorhawell
autogen docs
aab254f
@sorhawell
titles: join_asof -> Perform joins on nearest keys
686b12e
@sorhawell
autogen docs
89c1ccc
@etiennebacher
Copy link
Collaborator

The docs workflow fails because I pushed to the wrong branch earlier, I tried to remove the commit (it was b42c05e) but apparently it didn't work. Maybe you can just go back to the old code in docs.yml

sorhawell and others added 4 commits May 3, 2023 15:34
@sorhawell
revert docs.yml
5e95ade
@sorhawell
revert docs.yml #2
e465f1f
@sorhawell
revert docs.yml #3
0ee319a
@sorhawell @etiennebacher
Apply suggestions from code review
000d284 
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
etiennebacher
etiennebacher approved these changes May 3, 2023
View reviewed changes
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

I'm fine with this, looks like a very useful function, thanks

@etiennebacher
Copy link
Collaborator

One last thing, could you add a NEWS bullet point?

@sorhawell sorhawell merged commit b25e821 into main May 4, 2023
8 checks passed
@sorhawell sorhawell deleted the join_asof branch May 4, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etiennebacher etiennebacher etiennebacher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support join_asof
3 participants
@sorhawell @eitsupi @etiennebacher