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

fix(sql): avoid excessive inlining during Select merge #8825

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Mar 29, 2024

Another take on #8820 by introducing a complexity metric and only merge Select nodes if the outcome is less complex.

@kszucs kszucs force-pushed the fix-excessive-inlining branch from a47abed to 139e77d Compare March 29, 2024 12:39
ibis/backends/sql/rewrites.py Outdated Show resolved Hide resolved
ibis/expr/operations/core.py Outdated Show resolved Hide resolved
@cpcloud cpcloud force-pushed the fix-excessive-inlining branch from 1aee59b to a329506 Compare April 2, 2024 11:29
@cpcloud cpcloud added this to the 9.0 milestone Apr 2, 2024
@kszucs kszucs force-pushed the fix-excessive-inlining branch from a329506 to c76e730 Compare April 2, 2024 11:36
@kszucs kszucs marked this pull request as ready for review April 2, 2024 11:36
@cpcloud
Copy link
Member

cpcloud commented Apr 2, 2024

@NickCrews If you want to take this for a spin that would be great.

After this PR, the number of lines of SQL for 8.0.0 and in this PR are at the same order of magnitude, which IMO is enough of an improvement to merge it.

Here's a file with each version's generated SQL from the notebook provided in #8484:

8.0.0-nb.txt
8825-nb.txt

(they're .txt because GH doesn't allow uploading .sql files for some reason)

@kszucs kszucs force-pushed the fix-excessive-inlining branch 4 times, most recently from f60a664 to 0941713 Compare April 3, 2024 15:34
@kszucs kszucs changed the title refactor(sql): avoid excessive inlining refactor(sql): avoid excessive inlining during Select merge Apr 3, 2024
@kszucs kszucs changed the title refactor(sql): avoid excessive inlining during Select merge refactor(sql): avoid excessive inlining during Select merge Apr 3, 2024
@kszucs
Copy link
Member Author

kszucs commented Apr 3, 2024

@cpcloud I added docstrings to the complexity calculation.

@kszucs kszucs force-pushed the fix-excessive-inlining branch from 0941713 to e14a4bb Compare April 3, 2024 15:36
@kszucs kszucs force-pushed the fix-excessive-inlining branch from e14a4bb to 5cc3bb0 Compare April 4, 2024 09:51
@kszucs kszucs requested a review from cpcloud April 4, 2024 10:09
@cpcloud cpcloud added the refactor Issues or PRs related to refactoring the codebase label Apr 4, 2024
@cpcloud
Copy link
Member

cpcloud commented Apr 4, 2024

@kszucs Should this close #8484?

@NickCrews
Copy link
Contributor

Thank you so much for this! I just tested with the notebook in #8484

  • 8.0.0: fastest
  • this PR: ~10% slower than 8.0.0
  • main: 30+ times slower than 8.0.0

So this is definitely an improvement.

The theory of the implementation sounds right, even if the details go over my head a bit.

@cpcloud
Copy link
Member

cpcloud commented Apr 4, 2024

Ideally over time we can get back the 10%, but probably not before the 9.0 release.

I think this is the last major blocker for 9.0.

@cpcloud cpcloud changed the title refactor(sql): avoid excessive inlining during Select merge fix(sql): avoid excessive inlining during Select merge Apr 7, 2024
@cpcloud cpcloud merged commit ba931da into ibis-project:main Apr 7, 2024
86 checks passed
@cpcloud cpcloud deleted the fix-excessive-inlining branch April 7, 2024 11:24
@cpcloud cpcloud added performance Issues related to ibis's performance bug Incorrect behavior inside of ibis sql Backends that generate SQL labels Apr 7, 2024
gforsyth pushed a commit that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis performance Issues related to ibis's performance refactor Issues or PRs related to refactoring the codebase sql Backends that generate SQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants