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

[refine](Column)Disallow implicit conversion of ColumnPtr to IColumn* #45588

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Mryange
Copy link
Contributor

@Mryange Mryange commented Dec 18, 2024

What problem does this PR solve?

Previously, we allowed ColumnPtr to be directly converted to Column*:

    ColumnPtr column;  
    const IColumn* ptr = column;  

This can easily cause confusion.
For example, in the following code:

    ColumnPtr column;  
    const auto& const_column = check_and_get_column<ColumnConst>(column);  

The matched function is:

template <>  
const doris::vectorized::ColumnConst* check_and_get_column<doris::vectorized::ColumnConst>(  
        const IColumn* column)  

However, the actual type of const_column is:

const doris::vectorized::ColumnConst* const&

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

@Mryange
Copy link
Contributor Author

Mryange commented Dec 18, 2024

run buildall

Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In gensrc/script/gen_build_version.sh line 38:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.


In gensrc/script/gen_build_version.sh line 228:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.

For more information:
  https://www.shellcheck.net/wiki/SC2071 -- > is for string comparisons. Use ...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

@Mryange Mryange force-pushed the implicit-conversion-of-ColumnPtr branch from 7371ffe to 2cb39cf Compare December 18, 2024 07:53
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In gensrc/script/gen_build_version.sh line 38:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.


In gensrc/script/gen_build_version.sh line 228:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.

For more information:
  https://www.shellcheck.net/wiki/SC2071 -- > is for string comparisons. Use ...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

Copy link
Contributor

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

@Mryange
Copy link
Contributor Author

Mryange commented Dec 18, 2024

run buildall

Copy link
Contributor

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

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.88% (10133/26065)
Line Coverage: 29.78% (85149/285942)
Region Coverage: 28.83% (43678/151495)
Branch Coverage: 25.37% (22190/87460)
Coverage Report: http://coverage.selectdb-in.cc/coverage/2cb39cfc0026f0795c3fdd955d369e90d7fbb277_2cb39cfc0026f0795c3fdd955d369e90d7fbb277/report/index.html

@Mryange
Copy link
Contributor Author

Mryange commented Dec 18, 2024

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.88% (10134/26066)
Line Coverage: 29.80% (85218/285998)
Region Coverage: 28.84% (43694/151524)
Branch Coverage: 25.38% (22199/87482)
Coverage Report: http://coverage.selectdb-in.cc/coverage/2cb39cfc0026f0795c3fdd955d369e90d7fbb277_2cb39cfc0026f0795c3fdd955d369e90d7fbb277/report/index.html

#ifndef BE_TEST
[[deprecated("Implicit conversion should not be used. Please use get() explicitly.")]]
#endif
operator T*() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this code, no need to give warnings

@Mryange Mryange force-pushed the implicit-conversion-of-ColumnPtr branch from 2cb39cf to 073c2e5 Compare December 20, 2024 01:46
@Mryange
Copy link
Contributor Author

Mryange commented Dec 20, 2024

run buildall

Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In gensrc/script/gen_build_version.sh line 38:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.


In gensrc/script/gen_build_version.sh line 228:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.

For more information:
  https://www.shellcheck.net/wiki/SC2071 -- > is for string comparisons. Use ...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

@Mryange
Copy link
Contributor Author

Mryange commented Dec 20, 2024

run buildall

Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In gensrc/script/gen_build_version.sh line 38:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.


In gensrc/script/gen_build_version.sh line 228:
if [[ ${build_version_hotfix} > 0 ]]; then
                              ^-- SC2071 (error): > is for string comparisons. Use -gt instead.

For more information:
  https://www.shellcheck.net/wiki/SC2071 -- > is for string comparisons. Use ...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

yiguolei
yiguolei previously approved these changes Dec 20, 2024
Copy link
Contributor

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

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

PR approved by anyone and no changes requested.

@Mryange
Copy link
Contributor Author

Mryange commented Dec 20, 2024

run buildall

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Dec 20, 2024
@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.87% (10131/26062)
Line Coverage: 29.81% (85292/286148)
Region Coverage: 28.94% (43550/150500)
Branch Coverage: 25.44% (22171/87154)
Coverage Report: http://coverage.selectdb-in.cc/coverage/0335cae2064f4f9a3267cfcb70bd71f03d13180e_0335cae2064f4f9a3267cfcb70bd71f03d13180e/report/index.html

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17599	7707	7372	7372
q2	2048	187	177	177
q3	10627	1142	1233	1142
q4	10593	760	703	703
q5	7641	2794	2708	2708
q6	243	153	149	149
q7	1023	647	602	602
q8	9267	1897	1970	1897
q9	6614	6517	6456	6456
q10	7024	2341	2373	2341
q11	457	259	258	258
q12	433	221	227	221
q13	17758	2930	3011	2930
q14	246	222	216	216
q15	560	513	491	491
q16	666	595	595	595
q17	1000	571	543	543
q18	7359	6678	6766	6678
q19	1340	1032	918	918
q20	487	183	180	180
q21	4076	3334	3251	3251
q22	376	318	305	305
Total cold run time: 107437 ms
Total hot run time: 40133 ms

----- Round 2, with runtime_filter_mode=off -----
q1	7328	7264	7256	7256
q2	326	224	227	224
q3	2916	2802	2877	2802
q4	2084	1836	1807	1807
q5	5719	5684	5668	5668
q6	228	144	142	142
q7	2246	1845	1789	1789
q8	3412	3617	3555	3555
q9	8994	8890	8948	8890
q10	3629	3569	3538	3538
q11	593	508	516	508
q12	839	642	604	604
q13	10509	3104	3202	3104
q14	307	271	272	271
q15	581	513	523	513
q16	694	663	658	658
q17	1871	1630	1688	1630
q18	8367	7812	7761	7761
q19	1774	1470	1708	1470
q20	2116	1881	1892	1881
q21	5741	5638	5574	5574
q22	670	594	627	594
Total cold run time: 70944 ms
Total hot run time: 60239 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 197646 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 0335cae2064f4f9a3267cfcb70bd71f03d13180e, data reload: false

query1	1321	958	943	943
query2	6227	2448	2475	2448
query3	11184	4808	4870	4808
query4	33023	23367	23414	23367
query5	4047	465	466	465
query6	286	189	184	184
query7	4013	305	312	305
query8	317	250	258	250
query9	9501	2720	2706	2706
query10	466	278	260	260
query11	17788	14996	15218	14996
query12	158	111	106	106
query13	1574	447	439	439
query14	9291	7608	7916	7608
query15	288	207	202	202
query16	8126	471	493	471
query17	2146	619	580	580
query18	2159	315	325	315
query19	368	168	172	168
query20	134	116	120	116
query21	206	107	105	105
query22	4999	4644	4594	4594
query23	34675	34239	33854	33854
query24	10321	2515	2566	2515
query25	608	413	408	408
query26	1243	163	161	161
query27	2396	344	346	344
query28	7573	2499	2452	2452
query29	846	414	421	414
query30	227	153	155	153
query31	1061	845	839	839
query32	96	59	61	59
query33	757	344	293	293
query34	1152	568	527	527
query35	906	751	789	751
query36	1105	957	964	957
query37	143	77	81	77
query38	4192	4141	4456	4141
query39	1493	1490	1454	1454
query40	208	134	103	103
query41	45	44	42	42
query42	119	107	112	107
query43	567	499	490	490
query44	1243	856	841	841
query45	191	168	166	166
query46	1213	762	750	750
query47	2058	1907	1916	1907
query48	426	333	329	329
query49	948	399	416	399
query50	851	415	399	399
query51	7323	7183	7083	7083
query52	106	91	92	91
query53	281	194	189	189
query54	1141	420	420	420
query55	83	83	77	77
query56	259	242	245	242
query57	1302	1169	1166	1166
query58	249	219	219	219
query59	3281	3124	3141	3124
query60	288	272	257	257
query61	108	105	107	105
query62	891	717	682	682
query63	229	196	201	196
query64	3904	671	656	656
query65	3306	3256	3220	3220
query66	785	305	330	305
query67	16457	15879	15512	15512
query68	6153	553	550	550
query69	497	263	244	244
query70	1269	1120	1080	1080
query71	470	267	256	256
query72	6524	4096	4121	4096
query73	811	367	373	367
query74	10363	8808	8840	8808
query75	4143	2625	2685	2625
query76	4024	1066	1117	1066
query77	660	323	280	280
query78	10301	9393	9328	9328
query79	1460	615	604	604
query80	1109	429	429	429
query81	514	232	230	230
query82	668	123	119	119
query83	201	148	144	144
query84	293	73	69	69
query85	1367	310	301	301
query86	410	302	302	302
query87	4472	4467	4535	4467
query88	3569	2238	2206	2206
query89	429	294	293	293
query90	2098	198	191	191
query91	137	101	105	101
query92	61	50	50	50
query93	2210	548	551	548
query94	853	288	280	280
query95	352	264	302	264
query96	629	275	293	275
query97	2884	2653	2645	2645
query98	215	197	205	197
query99	1635	1319	1332	1319
Total cold run time: 305059 ms
Total hot run time: 197646 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 32.15 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 0335cae2064f4f9a3267cfcb70bd71f03d13180e, data reload: false

query1	0.04	0.03	0.05
query2	0.08	0.03	0.04
query3	0.23	0.07	0.08
query4	1.61	0.10	0.10
query5	0.41	0.41	0.43
query6	1.15	0.66	0.65
query7	0.02	0.01	0.01
query8	0.04	0.03	0.04
query9	0.60	0.48	0.50
query10	0.54	0.56	0.55
query11	0.14	0.11	0.11
query12	0.14	0.11	0.11
query13	0.62	0.61	0.60
query14	2.85	2.74	2.74
query15	0.91	0.83	0.84
query16	0.37	0.40	0.37
query17	1.00	1.05	1.06
query18	0.23	0.23	0.22
query19	1.89	1.86	2.02
query20	0.01	0.01	0.01
query21	15.37	0.59	0.58
query22	2.80	2.29	1.78
query23	16.93	1.05	0.70
query24	2.75	0.57	1.53
query25	0.29	0.24	0.14
query26	0.36	0.13	0.13
query27	0.04	0.04	0.04
query28	10.84	1.10	1.08
query29	12.68	3.24	3.19
query30	0.25	0.07	0.06
query31	2.85	0.39	0.38
query32	3.24	0.46	0.45
query33	3.13	3.01	3.14
query34	17.05	4.49	4.54
query35	4.59	4.51	4.54
query36	0.68	0.48	0.48
query37	0.10	0.06	0.06
query38	0.04	0.04	0.03
query39	0.03	0.02	0.03
query40	0.17	0.12	0.13
query41	0.08	0.02	0.03
query42	0.04	0.02	0.02
query43	0.04	0.03	0.03
Total cold run time: 107.23 s
Total hot run time: 32.15 s

@Mryange
Copy link
Contributor Author

Mryange commented Dec 22, 2024

run buildall

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17574	7537	7361	7361
q2	2056	184	179	179
q3	10561	1111	1201	1111
q4	10581	783	702	702
q5	7596	2785	2707	2707
q6	244	153	148	148
q7	998	643	595	595
q8	9265	1933	1946	1933
q9	6638	6421	6512	6421
q10	7057	2306	2276	2276
q11	472	265	262	262
q12	437	223	223	223
q13	17793	2986	3046	2986
q14	245	219	216	216
q15	571	529	503	503
q16	681	576	567	567
q17	998	578	541	541
q18	7317	6656	6843	6656
q19	1352	971	975	971
q20	466	178	178	178
q21	4072	3381	3186	3186
q22	371	322	327	322
Total cold run time: 107345 ms
Total hot run time: 40044 ms

----- Round 2, with runtime_filter_mode=off -----
q1	7208	7235	7193	7193
q2	333	232	233	232
q3	2946	2839	2916	2839
q4	2082	1894	1911	1894
q5	5665	5703	5687	5687
q6	232	139	144	139
q7	2252	1827	1796	1796
q8	3403	3562	3553	3553
q9	8949	9065	9025	9025
q10	3602	3566	3525	3525
q11	597	497	518	497
q12	818	583	577	577
q13	11718	3172	3165	3165
q14	310	272	281	272
q15	575	505	505	505
q16	703	654	676	654
q17	1863	1646	1599	1599
q18	8363	7710	7596	7596
q19	1753	1602	1526	1526
q20	2103	1878	1889	1878
q21	5635	5385	5436	5385
q22	695	559	581	559
Total cold run time: 71805 ms
Total hot run time: 60096 ms

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.87% (10130/26063)
Line Coverage: 29.80% (85278/286170)
Region Coverage: 28.93% (43547/150524)
Branch Coverage: 25.43% (22171/87174)
Coverage Report: http://coverage.selectdb-in.cc/coverage/0335cae2064f4f9a3267cfcb70bd71f03d13180e_0335cae2064f4f9a3267cfcb70bd71f03d13180e/report/index.html

@doris-robot
Copy link

TPC-DS: Total hot run time: 196792 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 0335cae2064f4f9a3267cfcb70bd71f03d13180e, data reload: false

query1	1289	968	948	948
query2	6258	2376	2242	2242
query3	11120	4936	4690	4690
query4	32906	23353	23639	23353
query5	3606	474	462	462
query6	290	205	196	196
query7	3992	305	309	305
query8	326	261	253	253
query9	9287	2677	2677	2677
query10	458	257	247	247
query11	17875	15301	15234	15234
query12	159	100	99	99
query13	1555	411	426	411
query14	10083	6836	6912	6836
query15	262	199	216	199
query16	8206	426	488	426
query17	1567	622	592	592
query18	2198	317	312	312
query19	292	162	174	162
query20	127	131	120	120
query21	212	111	111	111
query22	4924	4375	4410	4375
query23	34553	33856	34094	33856
query24	10433	2472	2517	2472
query25	647	396	402	396
query26	1193	160	158	158
query27	2369	351	347	347
query28	7663	2472	2445	2445
query29	865	455	415	415
query30	238	156	150	150
query31	1032	814	834	814
query32	94	57	58	57
query33	769	285	296	285
query34	1182	545	531	531
query35	908	759	780	759
query36	1125	959	986	959
query37	173	78	77	77
query38	4273	4282	4315	4282
query39	1534	1460	1457	1457
query40	206	105	105	105
query41	46	45	45	45
query42	123	110	103	103
query43	561	506	500	500
query44	1313	845	842	842
query45	195	172	170	170
query46	1214	734	754	734
query47	2035	1932	1905	1905
query48	434	338	319	319
query49	893	396	401	396
query50	840	402	396	396
query51	7450	7130	7280	7130
query52	112	93	97	93
query53	275	189	197	189
query54	1241	451	409	409
query55	91	81	81	81
query56	287	245	260	245
query57	1290	1193	1177	1177
query58	238	228	228	228
query59	3306	3169	3108	3108
query60	287	249	256	249
query61	105	106	104	104
query62	875	699	725	699
query63	225	197	192	192
query64	4059	686	758	686
query65	3324	3246	3286	3246
query66	804	322	331	322
query67	16253	15501	15521	15501
query68	4576	560	562	560
query69	446	269	269	269
query70	1254	1152	1152	1152
query71	507	272	255	255
query72	6294	4095	4100	4095
query73	795	364	365	364
query74	10248	8954	8815	8815
query75	3406	2628	2662	2628
query76	2771	1081	1157	1081
query77	394	280	280	280
query78	10091	9495	9586	9495
query79	1697	622	615	615
query80	1259	434	446	434
query81	502	247	231	231
query82	280	120	125	120
query83	198	146	144	144
query84	290	69	69	69
query85	1112	310	299	299
query86	345	312	275	275
query87	4753	4454	4345	4345
query88	4516	2184	2215	2184
query89	431	297	290	290
query90	2204	192	193	192
query91	140	106	104	104
query92	66	53	52	52
query93	3884	551	549	549
query94	934	288	288	288
query95	357	256	250	250
query96	649	282	280	280
query97	2883	2700	2703	2700
query98	226	200	199	199
query99	1569	1318	1294	1294
Total cold run time: 303202 ms
Total hot run time: 196792 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 33.63 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 0335cae2064f4f9a3267cfcb70bd71f03d13180e, data reload: false

query1	0.03	0.03	0.04
query2	0.07	0.04	0.03
query3	0.23	0.08	0.06
query4	1.62	0.10	0.11
query5	0.42	0.41	0.41
query6	1.16	0.65	0.65
query7	0.02	0.02	0.02
query8	0.04	0.03	0.03
query9	0.58	0.50	0.51
query10	0.54	0.57	0.56
query11	0.14	0.11	0.11
query12	0.15	0.11	0.11
query13	0.60	0.61	0.59
query14	2.76	2.73	2.82
query15	0.89	0.84	0.83
query16	0.39	0.38	0.38
query17	1.03	1.06	1.00
query18	0.22	0.22	0.21
query19	1.88	1.82	1.92
query20	0.01	0.01	0.02
query21	15.36	0.61	0.57
query22	2.79	2.65	2.10
query23	17.02	0.90	0.79
query24	2.93	1.52	1.87
query25	0.25	0.24	0.19
query26	0.33	0.14	0.14
query27	0.04	0.04	0.05
query28	9.63	1.11	1.08
query29	12.58	3.25	3.25
query30	0.24	0.07	0.07
query31	2.86	0.39	0.38
query32	3.25	0.48	0.47
query33	3.13	3.21	3.08
query34	17.11	4.48	4.52
query35	4.51	4.52	4.55
query36	0.67	0.47	0.48
query37	0.10	0.06	0.06
query38	0.05	0.04	0.03
query39	0.03	0.03	0.02
query40	0.16	0.12	0.12
query41	0.07	0.02	0.02
query42	0.03	0.02	0.02
query43	0.04	0.04	0.03
Total cold run time: 105.96 s
Total hot run time: 33.63 s

@Mryange
Copy link
Contributor Author

Mryange commented Dec 22, 2024

run p0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants