Skip to content

[fix](be) resolves some unused-raii and used-after-moved issues #29285

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
Merged
yiguolei merged 4 commits into from
Dec 30, 2023
Merged

[fix](be) resolves some unused-raii and used-after-moved issues #29285

yiguolei merged 4 commits into from
Dec 30, 2023

Conversation

@py023
Copy link
Contributor

@py023 py023 commented Dec 29, 2023

Proposed changes

This PR partially resolves #28965 by applying some trivial patches. Either by removing unncessary variable usage or using correct variable which has the data ownership. Similiar fixes are grouped in the same commit.

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

py023 added 4 commits December 27, 2023 17:11
@py023
[fix](config) unused raii
7b47a13 
Signed-off-by: pengyu <pengyu@selectdb.com>
@py023
fix(use-after-move) simple string moved
48db20e 
Signed-off-by: pengyu <pengyu@selectdb.com>
@py023
fix(used-after-moved): insignificant pointer reused
6bde018 
Signed-off-by: pengyu <pengyu@selectdb.com>
@py023
fix(used-after-moved): remove seldomly used tracelog
d52b7b4 
Signed-off-by: pengyu <pengyu@selectdb.com>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2023

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

yiguolei
yiguolei approved these changes Dec 29, 2023
View reviewed changes
Copy link
Contributor

@yiguolei yiguolei left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Dec 29, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2023

PR approved by anyone and no changes requested.

amorynan
amorynan reviewed Dec 30, 2023
View reviewed changes
be/src/http/action/config_action.cpp
std::string status(s.ok() ? "OK" : "BAD");
rapidjson::Value result;
result.SetObject();
rapidjson::Value(key.c_str(), key.size(), results.GetAllocator());
Copy link
Contributor

@amorynan amorynan Dec 30, 2023

Choose a reason for hiding this comment

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

is here no problems before ?

Copy link
Contributor Author

@py023 py023 Dec 30, 2023

Choose a reason for hiding this comment

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

No, IIUC it just creates an useless JSON value and drops it immediately.

amorynan
amorynan approved these changes Dec 30, 2023
View reviewed changes
Copy link
Contributor

@amorynan amorynan left a comment

Choose a reason for hiding this comment

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

LGTM

@yiguolei
Copy link
Contributor

yiguolei commented Dec 30, 2023

run buildall

@doris-robot
Copy link

doris-robot commented Dec 30, 2023

TeamCity be ut coverage result:
Function Coverage: 36.61% (8606/23506)
Line Coverage: 28.68% (70016/244086)
Region Coverage: 27.68% (36239/130940)
Branch Coverage: 24.38% (18520/75976)
Coverage Report: http://coverage.selectdb-in.cc/coverage/d52b7b4bc9cdf2eb5f1f6277871601bcbe92c170_d52b7b4bc9cdf2eb5f1f6277871601bcbe92c170/report/index.html

@doris-robot
Copy link

doris-robot commented Dec 30, 2023

TPC-H test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'

Tpch sf100 test result on commit d52b7b4bc9cdf2eb5f1f6277871601bcbe92c170, data reload: false

run tpch-sf100 query with default conf and session variables
q1	5442	5149	5181	5149
q2	401	153	158	153
q3	1492	1175	1139	1139
q4	1098	835	812	812
q5	3119	3086	3147	3086
q6	236	141	136	136
q7	970	572	528	528
q8	2174	2259	2214	2214
q9	6747	6693	6685	6685
q10	3207	3155	3134	3134
q11	351	224	227	224
q12	389	236	233	233
q13	4411	3651	3653	3651
q14	262	210	217	210
q15	613	551	560	551
q16	455	412	411	411
q17	1040	505	578	505
q18	7087	6767	6785	6767
q19	1646	1541	1598	1541
q20	614	326	353	326
q21	2925	2509	2490	2490
q22	387	315	315	315
Total cold run time: 45066 ms
Total hot run time: 40260 ms

run tpch-sf100 query with default conf and set session variable runtime_filter_mode=off
q1	5196	5072	5122	5072
q2	353	251	268	251
q3	3385	3300	3330	3300
q4	2181	2032	2028	2028
q5	5966	5940	5912	5912
q6	231	127	133	127
q7	2414	1984	1928	1928
q8	3578	3692	3668	3668
q9	9106	9051	9039	9039
q10	3895	3955	3940	3940
q11	583	474	493	474
q12	818	665	645	645
q13	3896	3194	3204	3194
q14	297	265	270	265
q15	603	557	558	557
q16	560	514	512	512
q17	2048	1813	1804	1804
q18	8778	8373	8364	8364
q19	1753	1716	1689	1689
q20	2283	2024	1985	1985
q21	5719	5512	5404	5404
q22	577	513	495	495
Total cold run time: 64220 ms
Total hot run time: 60653 ms

@doris-robot
Copy link

doris-robot commented Dec 30, 2023

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 47.22 seconds
stream load tsv: 563 seconds loaded 74807831229 Bytes, about 126 MB/s
stream load json: 19 seconds loaded 2358488459 Bytes, about 118 MB/s
stream load orc: 66 seconds loaded 1101869774 Bytes, about 15 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.1 seconds inserted 10000000 Rows, about 355K ops/s
storage size: 17188006561 Bytes

@yiguolei yiguolei merged commit e7d67e9 into apache:master Dec 30, 2023
HappenLee pushed a commit to HappenLee/incubator-doris that referenced this pull request Jan 12, 2024
@py023 @HappenLee
[fix](be) resolves some unused-raii and used-after-moved issues (apac…
7d3ce58 
…he#29285)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yiguolei yiguolei yiguolei approved these changes

+1 more reviewer

@amorynan amorynan amorynan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Multiple error reported by clang-tidy bugprone-*

4 participants

@py023 @yiguolei @doris-robot @amorynan