-
Notifications
You must be signed in to change notification settings - Fork 35
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
Unit test optimization toggles from LazyFrame #405
Conversation
Currently the unit test is doing nothing because there is no way to extract optimization toggles after setting them in functions like |
Hi @Sicheng-Pan thx for looking into this :) how about setting optstate on a lazyframe with internal functions and then check if actually set with this new getter ? |
@sorhawell Sounds good to me. I've already exported this getter function to R and I will write an internal setter as well. May be we could also make the setter public and allow the users to keep existing optimization settings in functions like |
How about having some global option defining if optstate should be set via an external setter or vi collect etc? Then the option could be used in collect. We should let default behavior match py-polars when there is no clear need otherwise. Some edge casesnto consider: What if two lazyframes are joined, what shouldhappen to optstate? How does collect know if a lazyframe settings should not be altered, because a user has already set them? |
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.
there is a handful of bugs and error handling in flow in LazyFrame__lazy.R
there could be more unit tests to tease out these errors
tests/testthat/test-lazy.R
Outdated
@@ -766,3 +766,18 @@ test_that("unnest", { | |||
to_data_frame() | |||
) | |||
}) | |||
|
|||
test_that("opt_toggles", { | |||
lf = pl$LazyFrame(mtcars)$select(pl$col("mpg") * 0.42) |
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.
try add some more unit tests for sink_parquet etc to confirm no errors
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've added a few more tests.
R/lazyframe__lazy.R
Outdated
self |> | ||
.pr$LazyFrame$optimization_toggle( | ||
if (isTRUE(inherit_optimization)) { | ||
self |
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 self is not being fetched
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.
Fixed.
R/lazyframe__lazy.R
Outdated
if (isTRUE(inherit_optimization)) { | ||
self | ||
} else { | ||
self$set_optimization_toggle( |
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.
any error in set_optimization_toggle will be raised and escaped the final unwrap
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.
consider using .pr$LazyFrame$set_opt... or wrap entire block in result() to catch any errors
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.
Fixed.
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.
thx I will review it soon
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.
Many thanks @Sicheng-Pan !
#325
Update nix flake and write the unit test for LazyFrame optimization toggles.