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.

Sign up for GitHub

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

Jump to bottom

refactor(api): remove unnecessary select from set operations #9438

Merged
merged 2 commits into from
Jun 25, 2024
Merged

refactor(api): remove unnecessary select from set operations #9438

merged 2 commits into from
Jun 25, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jun 25, 2024

Closes #5917.

@cpcloud cpcloud added this to the 9.2 milestone Jun 25, 2024
@cpcloud cpcloud added internals Issues or PRs related to ibis's internal APIs ux User experience related issues sql Backends that generate SQL labels Jun 25, 2024
jcrist
jcrist approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: (provided tests pass)

ibis/expr/types/relations.py Show resolved Hide resolved
@cpcloud cpcloud force-pushed the remove-select-from-unions branch 2 times, most recently from 621d15b to 9bfda9e Compare June 25, 2024 16:29
@cpcloud cpcloud changed the title refactor(api): remove select from unions refactor(api): remove unnecessary select from set operations Jun 25, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Jun 25, 2024

This should also give us even more breathing room for huge unions, since we can compile big chains of them without triggering the recursive sqlglot compilation of Select objects.

cpcloud
cpcloud commented Jun 25, 2024
View reviewed changes
ibis/tests/benchmarks/test_benchmarks.py
@@ -850,19 +850,20 @@ def test_column_access(benchmark, many_cols, getter):
benchmark(getter, many_cols)


@pytest.fixture(scope="module")
def many_tables():
@pytest.fixture(scope="module", params=[1000, 10000])
Copy link
Member Author

@cpcloud cpcloud Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An expression with 10k unions is definitely hitting the upper limit of interactive latency acceptability (about 9s on a gcp instance with 64 cores and 96G of RAM).

@cpcloud
Copy link
Member Author

cpcloud commented Jun 25, 2024

Clouds are passing:

cloud in 🌐 falcon in …/ibis on  remove-select-from-unions is 📦 v9.1.0 via 🐍 v3.10.14 via ❄️   impure (ibis-3.10.14-env)
❯ pytest -m 'bigquery or snowflake' --dist loadgroup -q -n 8 --snapshot-update
bringing up nodes...
............................................................................................................................................................x.ssssssssssssssssssssssxxxx..x..xx......xx [  5%]
.....xx......xx.x.xx..x......x.....xx....x........x.....x....xx..x...x..x...x....x...x......x............x.x..........x.x..xx........x.........x......sx....xx......x...........x...............s...... [ 10%]
x.........s.......x....s.........xx.xxxxxxxxxx.x.x.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.x.xxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxx.xxxxxxxxxxxx.xxx.x...xxxx.........xxx..x.....x.... [ 16%]
......x.........x....xx........x..xx...x...x..........x.....................x................x..x.xxx.....x.......x...........x...........x........x..........x.xx...x.xxx...x...xxx...x.xxx.xxx.xx.x.. [ 21%]
.xx..xx..x..x.....x.x...xx....................................x...................x.................................................................x.x...............................s................ [ 27%]
..............................s....................s.................................x.x......x...........x...........x...s......x...x........x.....................x..x.......x.....x....x...x.x...xx. [ 32%]
......x...x..........xx.x........x.............x........x....xx.x...x..........xx.....x...x..x........xx..............x....x...xxxx....x.........xxx....x.x.x.x.xx.xxx..xx.xx.x.x.x.....x............x. [ 38%]
x...xx.................x.x..........x.........x...............x....................x......x......x............x.x...xx..xx.......x.x...x.x..x............x.................x......x.xx................. [ 43%]
......x.............................x......x........................x...........x.......................xx.......xxx..x......x..x.x...xxx.x............x..x......xx...............xx................... [ 49%]
......x....x.....x......x...................xx........x.........x.....x.x...........x..x....x..........................x......x.....s...............x.......x..xx.....xxx..x.xxx.x...xx.x....x.x.xxx... [ 54%]
xxxx......xxxxx.x.xxx..xx.x.xx.xx.x.xx...x..xxx..xxx...xx.x.x........xx.x.x...xx..x..xxx...x........x....x..x.x..x.......x....xx.x......xx.....x.........x.......xx..........x.......x..........x....xx [ 59%]
..x....x..........x............x..x...x....x...xx............x...........x..x..xx....x..........x..x...........x...xx..................x............x.x............x...................x....x.......... [ 65%]
...........x...x.....................x...x.x.............x.......x.............x..x......x........s........s.........................................................s...............x...............x. [ 70%]
..x.x..........x...xx..x.x......x..xx...x...x.s................x...x..........x.....................................x.....xx........................................................................... [ 76%]
........................s..............x..x.....x.........xx.xxxxxxx..xx...x..........x...........x...........xx.....x..xx........x...........x..........x...................x..........x............x. [ 81%]
.........x...................x.x.............x...x.x......................x.x........xx................................................................................x............x.........x........ [ 87%]
.x....................s..s............................................................................................................................................................................. [ 92%]
.......................................................................x............................................................................................................................... [ 98%]
.....................s.................................xx.......s........                                                                                                                               [100%]
3063 passed, 40 skipped, 552 xfailed in 641.69s (0:10:41)

@cpcloud cpcloud enabled auto-merge (squash) June 25, 2024 17:12
@cpcloud cpcloud merged commit 88a2785 into ibis-project:main Jun 25, 2024
76 checks passed
@cpcloud cpcloud deleted the remove-select-from-unions branch June 25, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcrist jcrist jcrist approved these changes

@gforsyth gforsyth Awaiting requested review from gforsyth

@kszucs kszucs Awaiting requested review from kszucs

@ncclementi ncclementi Awaiting requested review from ncclementi

Assignees
No one assigned
Labels
internals Issues or PRs related to ibis's internal APIs sql Backends that generate SQL ux User experience related issues
Projects
None yet
Milestone
9.2
Development

Successfully merging this pull request may close these issues.

refactor(compilers): figure out how to avoid generating projections for every union
2 participants
@cpcloud @jcrist