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

perf(sql): don't compile pretty sql by default #8616

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Mar 11, 2024

We have been compiling pretty formatted SQL queries so far. While this was handy, pretty formatting doesn't give any value when directly passed to a backend, it mostly matters when presented to users.

Compiling a big query like the one provided in #8484 takes 3.83s with pretty=True while it takes almost half the time 2.17s with pretty=False.

Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@cpcloud
Copy link
Member

cpcloud commented Mar 11, 2024

Does the use of formatted SQL have a meaningful impact on SQL generation performance?

@kszucs kszucs force-pushed the compile-pretty branch 3 times, most recently from 5a3bdc6 to 4425343 Compare March 11, 2024 13:52
@kszucs
Copy link
Member Author

kszucs commented Mar 11, 2024

Depends on the expression:

While it is not significant in the first case, it is in the second.

@kszucs kszucs force-pushed the compile-pretty branch 2 times, most recently from e0dfb68 to 82cea10 Compare March 13, 2024 18:03
@kszucs kszucs marked this pull request as ready for review March 13, 2024 19:18
@kszucs kszucs changed the title perf(sql): don't compile pretty sql by default perf(sql): don't compile pretty sql by default Mar 14, 2024
@kszucs kszucs requested a review from cpcloud March 14, 2024 11:25
@cpcloud cpcloud added this to the 9.0 milestone Mar 14, 2024
@cpcloud cpcloud added performance Issues related to ibis's performance sql Backends that generate SQL labels Mar 14, 2024
@cpcloud cpcloud merged commit af402f9 into ibis-project:main Mar 19, 2024
78 checks passed
@cpcloud cpcloud deleted the compile-pretty branch March 19, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues related to ibis's performance sql Backends that generate SQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants