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

[fix](runtime filter) Disable build_bf_exactly if `sync_filter_size… #44169

Merged
merged 1 commit into from
Nov 19, 2024
Merged

[fix](runtime filter) Disable build_bf_exactly if `sync_filter_size… #44169

merged 1 commit into from
Nov 19, 2024

Conversation

Gabriel39
Copy link
Contributor

@Gabriel39 Gabriel39 commented Nov 18, 2024 •
edited
Loading

…` is disabled

What problem does this PR solve?

When a bloom filter has multiple targets, it should use a size which is estimated by FE.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.01% (9899/26042)
Line Coverage: 29.18% (82727/283547)
Region Coverage: 28.31% (42492/150076)
Branch Coverage: 24.88% (21544/86598)
Coverage Report: http://coverage.selectdb-in.cc/coverage/748807fb9a4e285af48cbd25d94555df8e3862d3_748807fb9a4e285af48cbd25d94555df8e3862d3/report/index.html

HappenLee
HappenLee reviewed Nov 19, 2024
View reviewed changes
be/src/exprs/runtime_filter.cpp
@@ -1355,20 +1355,18 @@ Status IRuntimeFilter::init_with_desc(const TRuntimeFilterDesc* desc, const TQue
params.runtime_bloom_filter_max_size = options->__isset.runtime_bloom_filter_max_size
? options->runtime_bloom_filter_max_size
: 0;
// We build runtime filter by exact distinct count iff three conditions are met:
auto sync_filter_size = desc->__isset.sync_filter_size && desc->sync_filter_size;
// We build runtime filter by exact distinct count if all of 3 conditions are met:
Copy link
Contributor

Choose a reason for hiding this comment

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

should add broadcast join

HappenLee
HappenLee approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@HappenLee HappenLee left a comment

Choose a reason for hiding this comment

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

LGTM

@Gabriel39
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions bot added approved Indicates a PR has been approved by one committer. reviewed labels Nov 19, 2024
@github-actions GitHub Actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TPC-H: Total hot run time: 45343 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 7332f5bc2f590217107847da679b48b86f1297e8, data reload: false

------ Round 1 ----------------------------------
q1	17592	7458	7344	7344
q2	2225	1165	1173	1165
q3	9972	1705	1287	1287
q4	10374	807	680	680
q5	7616	2752	2691	2691
q6	243	148	146	146
q7	990	632	604	604
q8	9379	2381	2373	2373
q9	6886	6516	6474	6474
q10	7081	2293	2360	2293
q11	485	255	272	255
q12	535	216	227	216
q13	17787	3083	3087	3083
q14	256	206	215	206
q15	565	529	536	529
q16	705	584	597	584
q17	988	547	583	547
q18	7438	6744	6695	6695
q19	1341	1089	941	941
q20	2929	2702	2719	2702
q21	4021	3347	3194	3194
q22	1399	1334	1349	1334
Total cold run time: 110807 ms
Total hot run time: 45343 ms

----- Round 2, with runtime_filter_mode=off -----
q1	7342	7237	7213	7213
q2	326	226	227	226
q3	2908	2821	2790	2790
q4	1978	1781	1715	1715
q5	5391	5439	5495	5439
q6	221	136	133	133
q7	2097	1689	1686	1686
q8	3301	3464	3442	3442
q9	8555	8596	8550	8550
q10	3511	3465	3444	3444
q11	585	487	492	487
q12	797	599	579	579
q13	14495	3064	3078	3064
q14	300	254	262	254
q15	566	518	507	507
q16	648	643	627	627
q17	1824	1578	1579	1578
q18	7903	7527	7373	7373
q19	1690	1458	1520	1458
q20	2075	1841	1822	1822
q21	5258	5219	5186	5186
q22	619	544	583	544
Total cold run time: 72390 ms
Total hot run time: 58117 ms

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.01% (9899/26042)
Line Coverage: 29.20% (82802/283550)
Region Coverage: 28.33% (42508/150067)
Branch Coverage: 24.89% (21554/86592)
Coverage Report: http://coverage.selectdb-in.cc/coverage/7332f5bc2f590217107847da679b48b86f1297e8_7332f5bc2f590217107847da679b48b86f1297e8/report/index.html

@doris-robot
Copy link

TPC-H: Total hot run time: 44986 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 7332f5bc2f590217107847da679b48b86f1297e8, data reload: false

------ Round 1 ----------------------------------
q1	17604	7503	7356	7356
q2	2237	1168	1182	1168
q3	9960	1194	1229	1194
q4	10226	746	752	746
q5	7590	2731	2630	2630
q6	239	146	146	146
q7	997	632	601	601
q8	9363	2380	2381	2380
q9	6743	6408	6481	6408
q10	7088	2305	2296	2296
q11	474	260	254	254
q12	525	220	217	217
q13	17791	3059	3014	3014
q14	253	208	212	208
q15	571	537	516	516
q16	678	595	581	581
q17	994	532	527	527
q18	7472	6738	6792	6738
q19	1338	993	923	923
q20	2901	2700	2688	2688
q21	4009	3299	3074	3074
q22	1388	1357	1321	1321
Total cold run time: 110441 ms
Total hot run time: 44986 ms

----- Round 2, with runtime_filter_mode=off -----
q1	7293	7279	7267	7267
q2	335	223	231	223
q3	2937	2799	2797	2797
q4	2015	1705	1741	1705
q5	5410	5442	5448	5442
q6	217	138	134	134
q7	2104	1703	1723	1703
q8	3257	3388	3396	3388
q9	8609	8547	8620	8547
q10	3506	3476	3438	3438
q11	582	501	492	492
q12	775	619	588	588
q13	13663	3055	3056	3055
q14	286	263	266	263
q15	553	513	513	513
q16	699	630	624	624
q17	1846	1599	1577	1577
q18	7851	7484	7455	7455
q19	1664	1557	1539	1539
q20	2051	1839	1840	1839
q21	5376	5227	5132	5132
q22	604	543	564	543
Total cold run time: 71633 ms
Total hot run time: 58264 ms

@Gabriel39 Gabriel39 merged commit 0cbad3b into apache:master Nov 19, 2024
26 of 29 checks passed
Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Nov 21, 2024
@Gabriel39
[fix](runtime filter) Disable build_bf_exactly if `sync_filter_size… (
d3a11f2 
apache#44169)

…` is disabled

When a bloom filter has multiple targets, it should use a size which is
estimated by FE.
Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Nov 21, 2024
@Gabriel39
[fix](runtime filter) Disable build_bf_exactly if `sync_filter_size… (
28b935f 
apache#44169)

…` is disabled

When a bloom filter has multiple targets, it should use a size which is
estimated by FE.
Gabriel39 added a commit that referenced this pull request Nov 26, 2024
@Gabriel39
[branch-3.0](pick) pick #44169 #44367 (#44394)
a5a2a56 
pick #44169 #44367
@BiteTheDDDDt BiteTheDDDDt mentioned this pull request Nov 28, 2024
Merged
16 tasks
BiteTheDDDDt added a commit that referenced this pull request Dec 3, 2024
@BiteTheDDDDt
[Bug](runtime-filter) fix wrong build_bf_exactly when sync filter siz…
0d48863 
…e disabled (#44716)

### What problem does this PR solve?
fix wrong build_bf_exactly when sync filter size disabled

introduced by #44169
BiteTheDDDDt added a commit that referenced this pull request Jan 14, 2025
@BiteTheDDDDt
[Bug](runtime-filter) fix wrong build_bf_exactly when sync filter siz…
7a1de39 
…e disabled (#44716)

fix wrong build_bf_exactly when sync filter size disabled

introduced by #44169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HappenLee HappenLee HappenLee approved these changes

@BiteTheDDDDt BiteTheDDDDt BiteTheDDDDt approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by one committer. dev/3.0.4-merged reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Gabriel39 @doris-robot @BiteTheDDDDt @HappenLee