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

feat(optimizer): refine the conversion from Join to Dynamic Filter #5789

Merged
merged 10 commits into from
Oct 12, 2022

Conversation

BugenZhao
Copy link
Member

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR refines the conversion from Join to Dynamic Filter.

  • support all scalar inner sides
  • ensure the output indices only from the outer side
  • fix output indices column mapping

Also add some planner tests.

Note that some patterns can not be planned now due to pk derivation of hash join. Check the issue below.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
…port-more-patterns

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao requested review from jon-chuang, chenzl25 and xxchan and removed request for jon-chuang and chenzl25 October 11, 2022 11:52
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #5789 (857d133) into main (6d36139) will increase coverage by 0.00%.
The diff coverage is 95.00%.

@@           Coverage Diff           @@
##             main    #5789   +/-   ##
=======================================
  Coverage   75.01%   75.01%           
=======================================
  Files         916      916           
  Lines      143471   143495   +24     
=======================================
+ Hits       107621   107649   +28     
+ Misses      35850    35846    -4     
Flag Coverage Δ
rust 75.01% <95.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...c/frontend/src/optimizer/plan_node/logical_join.rs 92.83% <91.17%> (+0.23%) ⬆️
...d/src/optimizer/plan_node/stream_dynamic_filter.rs 100.00% <100.00%> (ø)
.../src/executor/managed_state/aggregation/extreme.rs 95.94% <0.00%> (+0.13%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +27 to +33
- name: |
With Top-1 on inner side
TODO: currently not possible due to https://github.com/risingwavelabs/risingwave/issues/5764
before:
- create_tables
sql: |
with max_v2 as (select v2 max from t2 order by v2 desc limit 1) select v1 from t1, max_v2 where v1 > max;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some off-topic idea. Maybe we can support more range conditions than simple >, >=, <, <=.
e.g. between

 with max_v2 as (select max(v2) max from t2) select v1 from t1, max_v2 where v1 between max - 100 and max + 100;

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, we can support arbitrary range in the future.

Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

LGTM, btw the LogicalJoin::to_stream is too long... It should be split into multiple functions. Maybe I will do it later

@mergify mergify bot merged commit db5415f into main Oct 12, 2022
@mergify mergify bot deleted the bz/dynamic-filter-support-more-patterns branch October 12, 2022 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants