Skip to content

Conversation

@koarz
Copy link
Contributor

@koarz koarz commented Mar 14, 2024

Proposed changes

Issue Number: close #xxx

Previously, due to a mistake on my part, I changed the behaviour of the repeat function, but I didn't realise the problem

            int repeat = 0;
            repeat = std::min<int>(repeats[i], repeat_max_num);

            if (repeat <= 0) {
                StringOP::push_empty_string(i, res_data, res_offsets);
            } else if (repeat * size > DEFAULT_MAX_STRING_SIZE) {
                StringOP::push_null_string(i, res_data, res_offsets, null_map);
            } else {
                for (int j = 0; j < repeat; ++j) {
                    buffer.append(raw_str, raw_str + size);
                }
                StringOP::push_value_string(std::string_view(buffer.data(), buffer.size()), i,
                                            res_data, res_offsets);
            }

You can see that in the case of repeat * size > DEFAULT_MAX_STRING_SIZE we need to output null, but due to an oversight on my part I had defaulted to repeat being less than or equal to repeat_max_num, so I removed this judgement resulting in a change in behaviour
So we need the revert function

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...

@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

@koarz
Copy link
Contributor Author

koarz commented Mar 14, 2024

run buildall

@morrySnow
Copy link
Contributor

please add some description to explain why need revert

@github-actions
Copy link
Contributor

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

@koarz
Copy link
Contributor Author

koarz commented Mar 14, 2024

please add some description to explain why need revert

Description has been added

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

@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 Mar 14, 2024
@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17646	4201	4121	4121
q2	2010	144	138	138
q3	10616	1112	892	892
q4	7780	723	705	705
q5	7457	2724	2738	2724
q6	182	119	129	119
q7	1162	801	780	780
q8	9377	2008	1984	1984
q9	7164	6411	6359	6359
q10	8496	3438	3639	3438
q11	428	219	217	217
q12	653	288	292	288
q13	17795	2862	2849	2849
q14	270	236	241	236
q15	502	466	454	454
q16	483	401	389	389
q17	940	524	626	524
q18	7039	6384	6352	6352
q19	1562	1433	1477	1433
q20	538	290	273	273
q21	6192	3473	3537	3473
q22	365	291	308	291
Total cold run time: 108657 ms
Total hot run time: 38039 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4115	4121	4072	4072
q2	316	222	222	222
q3	2947	2810	2764	2764
q4	1836	1530	1544	1530
q5	5211	5241	5237	5237
q6	188	113	115	113
q7	2162	1834	1803	1803
q8	3131	3267	3246	3246
q9	8560	8478	8489	8478
q10	3637	3670	3644	3644
q11	528	444	440	440
q12	716	549	554	549
q13	16910	2851	2830	2830
q14	281	243	253	243
q15	480	435	448	435
q16	445	396	418	396
q17	1723	1480	1448	1448
q18	7472	7146	7148	7146
q19	1627	1540	1555	1540
q20	1896	1704	1691	1691
q21	4740	4692	4818	4692
q22	527	460	476	460
Total cold run time: 69448 ms
Total hot run time: 52979 ms

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 35.01% (8572/24486)
Line Coverage: 26.76% (69455/259589)
Region Coverage: 26.03% (36077/138592)
Branch Coverage: 22.98% (18417/80144)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ee42672ab9633bc38894292ebab40ca1fb9fbcd3_ee42672ab9633bc38894292ebab40ca1fb9fbcd3/report/index.html

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.

Add a regression-test for > max repeat num, to make sure the behavior is stable.

@zhangstar333
Copy link
Contributor

zhangstar333 commented Mar 14, 2024

in PR #32219, will report error if greater than max repeat num.
could merge this revert pr firstly.

@zclllyybb
Copy link
Contributor

Add a regression-test for > max repeat num, to make sure the behavior is stable.

added in https://github.com/apache/doris/pull/32219/files. we merge both two prs

@zclllyybb
Copy link
Contributor

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 34.95% (8573/24527)
Line Coverage: 26.66% (69481/260654)
Region Coverage: 25.95% (36088/139074)
Branch Coverage: 22.90% (18429/80460)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ee42672ab9633bc38894292ebab40ca1fb9fbcd3_ee42672ab9633bc38894292ebab40ca1fb9fbcd3/report/index.html

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17642	4380	4108	4108
q2	2036	156	151	151
q3	10726	1094	911	911
q4	7603	727	717	717
q5	7490	2689	2680	2680
q6	188	122	121	121
q7	1193	828	806	806
q8	9411	2004	2001	2001
q9	7142	6484	6448	6448
q10	8531	3521	3609	3521
q11	433	231	216	216
q12	625	301	284	284
q13	17792	2843	2880	2843
q14	273	248	263	248
q15	496	450	451	450
q16	515	394	396	394
q17	947	628	549	549
q18	7119	6608	6454	6454
q19	4279	1410	1441	1410
q20	537	293	275	275
q21	6142	3565	3458	3458
q22	380	312	307	307
Total cold run time: 111500 ms
Total hot run time: 38352 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4209	4062	4064	4062
q2	320	225	220	220
q3	2970	2841	2863	2841
q4	1875	1559	1518	1518
q5	5214	5231	5253	5231
q6	203	113	114	113
q7	2256	1848	1845	1845
q8	3139	3294	3297	3294
q9	8555	8535	8596	8535
q10	3732	3660	3649	3649
q11	547	469	430	430
q12	745	554	563	554
q13	16913	2860	2814	2814
q14	275	262	262	262
q15	489	446	460	446
q16	453	403	409	403
q17	1755	1478	1467	1467
q18	7602	7359	7083	7083
q19	1595	1554	1521	1521
q20	1905	1722	1729	1722
q21	4879	4649	4757	4649
q22	524	445	460	445
Total cold run time: 70155 ms
Total hot run time: 53104 ms

@yiguolei yiguolei merged commit 01f924b into apache:master Mar 15, 2024
@koarz koarz deleted the fix branch March 22, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants