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(optimizer): fix distribution satisfy condition #2447

Merged
merged 11 commits into from
May 12, 2022

Conversation

st1page
Copy link
Contributor

@st1page st1page commented May 12, 2022

What's changed and what's your intention?

HashShard(a,b) means no records with same (a,b) on the different shards.
HashShard(a) means no records same (a) on the different shards.
then HashShard(a) satisfy HashShard(a, b).

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

#2442
#2454

Sorry, something went wrong.

@st1page st1page requested a review from skyzh May 12, 2022 04:58
@skyzh
Copy link
Contributor

skyzh commented May 12, 2022

Will this possibly fix the issue of left semi join TPC-Hs? cc @yuhao-su

@st1page st1page added type/fix Bug fix component/optimizer Query optimization. bug bash labels May 12, 2022
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM, need fix conflict.

@skyzh
Copy link
Contributor

skyzh commented May 12, 2022

The plan of Q18, Q20 gets changed. Maybe we can try them after this PR gets merged.

@yuhao-su
Copy link
Contributor

Will this possibly fix the issue of left semi join TPC-Hs? cc @yuhao-su

It's certainly possible since a incorrect plan can affect a lot. I'll try those tpchs see if they work/

@st1page st1page enabled auto-merge (squash) May 12, 2022 05:20
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #2447 (b846242) into main (7f02fda) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2447      +/-   ##
==========================================
+ Coverage   71.28%   71.31%   +0.02%     
==========================================
  Files         688      688              
  Lines       88174    88250      +76     
==========================================
+ Hits        62855    62932      +77     
+ Misses      25319    25318       -1     
Flag Coverage Δ
rust 71.31% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rontend/src/optimizer/plan_node/batch_hash_join.rs 91.66% <100.00%> (+0.75%) ⬆️
...ntend/src/optimizer/plan_node/eq_join_predicate.rs 87.35% <100.00%> (+1.82%) ⬆️
...c/frontend/src/optimizer/plan_node/logical_join.rs 87.57% <100.00%> (+0.13%) ⬆️
...rc/frontend/src/optimizer/property/distribution.rs 85.36% <100.00%> (ø)
src/frontend/src/scheduler/execution/query.rs 67.32% <100.00%> (+2.47%) ⬆️
src/frontend/src/scheduler/plan_fragmenter.rs 95.02% <100.00%> (+0.32%) ⬆️
src/connector/src/filesystem/file_common.rs 80.80% <0.00%> (+0.44%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

st1page and others added 5 commits May 12, 2022 16:20

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@st1page st1page merged commit f72173a into main May 12, 2022
@st1page st1page deleted the sts/optimizer_fix_dist_satisfy branch May 12, 2022 09:25
@skyzh
Copy link
Contributor

skyzh commented May 12, 2022

Time to try some more TPC-H queries 🥵🥵 cc @yuhao-su

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/optimizer Query optimization. type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants