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: Fix join cost estimates #3831

Merged
merged 3 commits into from
Feb 28, 2025
Merged

Conversation

desmondcheongzx
Copy link
Contributor

@desmondcheongzx desmondcheongzx commented Feb 19, 2025

This PR fixes join cost estimates in the following ways:

  • Uses number of rows instead of size in bytes for making probe side decisions
  • Decreases the aggressiveness of cardinality reductions when there is an is not NULL filter
  • Decreases the aggressiveness of inequality filters, especially since these often appear as pairs of a "between" clause
  • Increases the aggressiveness of exact equality filters.
  • For join edges where there are more than one join condition, adjust cost estimations so that we compute the total domain as |(key1, key2, ...)| ~= min(|left side|, |right side|).
    With multiple join conditions, we know that the join is not a pk-fk join on each join key. Rather, it's a pk-fk join on the tuple of join keys, which we estimate as the cardinality of the smaller table of the join.

@desmondcheongzx desmondcheongzx force-pushed the desmond/coup-de-grace-join-order branch from a8c4fcc to aa56df8 Compare February 19, 2025 22:48
Copy link

codspeed-hq bot commented Feb 19, 2025

CodSpeed Performance Report

Merging #3831 will degrade performances by 27.56%

Comparing desmond/coup-de-grace-join-order (b246bd5) with main (b830647)

Summary

⚡ 1 improvements
❌ 3 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_tpch[1-in-memory-native-10] 213 ms 284.2 ms -25.08%
test_tpch[1-in-memory-native-5] 321.6 ms 382.7 ms -15.97%
test_tpch_sql[1-in-memory-native-10] 212.3 ms 293.1 ms -27.56%
test_tpch_sql[1-in-memory-native-8] 148.3 ms 130.1 ms +13.99%

@desmondcheongzx desmondcheongzx force-pushed the desmond/coup-de-grace-join-order branch from aa56df8 to fb088f8 Compare February 20, 2025 01:46
@desmondcheongzx desmondcheongzx force-pushed the desmond/coup-de-grace-join-order branch from fb088f8 to e6e0e95 Compare February 20, 2025 01:48
@desmondcheongzx desmondcheongzx changed the title impl join order perf: Fix join cost estimates Feb 20, 2025
@github-actions github-actions bot added the perf label Feb 20, 2025
@desmondcheongzx desmondcheongzx enabled auto-merge (squash) February 25, 2025 18:57
@desmondcheongzx desmondcheongzx merged commit 2351ba4 into main Feb 28, 2025
47 of 48 checks passed
@desmondcheongzx desmondcheongzx deleted the desmond/coup-de-grace-join-order branch February 28, 2025 04:10
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.64%. Comparing base (a42e55a) to head (b246bd5).
Report is 32 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3831      +/-   ##
==========================================
- Coverage   77.96%   77.64%   -0.32%     
==========================================
  Files         752      762      +10     
  Lines       95181    98720    +3539     
==========================================
+ Hits        74208    76652    +2444     
- Misses      20973    22068    +1095     
Files with missing lines Coverage Δ
src/daft-dsl/src/expr/mod.rs 74.90% <ø> (-1.61%) ⬇️
src/daft-local-execution/src/pipeline.rs 89.23% <100.00%> (ø)
...tion/rules/reorder_joins/brute_force_join_order.rs 99.81% <100.00%> (+0.03%) ⬆️
...src/optimization/rules/reorder_joins/join_graph.rs 91.14% <100.00%> (+0.19%) ⬆️

... and 147 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants