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.

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

Implement lazyframe profiling and optimization toggles #323

Merged
merged 24 commits into from
Aug 8, 2023

Conversation

Sicheng-Pan
Copy link
Collaborator

@sorhawell
Copy link
Collaborator

we are missing a unit test. py-polars do not always unit test this either. We need to extract the curret settings from a LazyFrame / logicalplan. I have previous found bugs in rust-polars by testing such optimization settings.

This was referenced Jul 17, 2023
@sorhawell
Copy link
Collaborator

sorhawell commented Jul 17, 2023

@Sicheng-Pan I'm ok with merging this PR now.

I think you mentioned you wanted to allow both the setting opt-toggles via collect as in py-polars, but that it also makes sense to se individually as in rust-polars. I'm agree with that, if you want to add that back in.

we can just postpone the unit tests until #324 has been merged

Merge branch 'main' into lazy_profile
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@sorhawell
Copy link
Collaborator

sorhawell commented Aug 4, 2023

This PR is needed for #343 due to opt-toogles + polars CSE feature + robj_to!(i32, robj)

@Sicheng-Pan
Also I realized that profile and fetch do collect like collect , but each of this functions need individual support for R_function. If using the plain rust .profile() or .fetch() method then R fn .map() is not supported.

Refactored and added two internal functions collect_with_r_func_support and collect_with_r_func_support. It should be easy to later also add fetch_with_r_func_support

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 could only review the R part, need someone else for the Rust part

R/lazyframe__lazy.R Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R 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
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
tests/testthat/test-lazy_profile.R Outdated Show resolved Hide resolved
@sorhawell sorhawell requested a review from eitsupi August 4, 2023 15:37
@sorhawell
Copy link
Collaborator

@etiennebacher @eitsupi I think this PR is ready to go. It is needed for the Sink PR #343

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 just did minor doc changes. @sorhawell since you and @Sicheng-Pan have worked on the Rust part I guess it's already reviewed.

I'm just waiting the CI to pass before merging, thank you both!

@etiennebacher etiennebacher merged commit b49e629 into main Aug 8, 2023
11 checks passed
@etiennebacher etiennebacher deleted the lazy_profile branch August 8, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants