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

refactor!: remove the eager option from <SQLContext>$execute() #719

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Jan 20, 2024

I think it is not very desirable for the return type to change depending on the value of the argument.
It is safe to always return a LazyFrame, since it costs very little to build one.

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 don't understand why this change is made: it's already eager = FALSE by default so we don't have the risk of accidentally collecting, and it would divert from the py-polars API (which is fine to do sometimes but here I just don't see why).

Do you have some usecases where this argument was a problem?

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 20, 2024

I don't understand why this change is made: it's already eager = FALSE by default so we don't have the risk of accidentally collecting, and it would divert from the py-polars API (which is fine to do sometimes but here I just don't see why).

Do you have some usecases where this argument was a problem?

  • I think the act of changing the return type depending on the argument is generally not a good thing (it should separate the function).
  • This option merely means that $collect() can be omitted, and by complicating the function, it increases the number of test cases and makes it easier to make bugs. (In fact, there was no test case here.)

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.

Alright, fine for me, thanks

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 20, 2024

Thanks for your review!

@eitsupi eitsupi merged commit 5072905 into main Jan 20, 2024
17 checks passed
@eitsupi eitsupi deleted the simplify-sql-execute branch January 20, 2024 11:11
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.

None yet

2 participants