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

[fix](schema change) resolve the use count check of source logical column #33932

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

AshinGau
Copy link
Member

@AshinGau AshinGau commented Apr 21, 2024

Proposed changes

Fix error like:

8# google::LogMessageFatal::~LogMessageFatal() in /mnt/hdd01/ci/master-deploy/be/lib/doris_be
 9# doris::vectorized::Block::clear_column_data(int) in /mnt/hdd01/ci/master-deploy/be/lib/doris_be
10# doris::vectorized::ParquetReader::get_next_block(doris::vectorized::Block*, unsigned long*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/format/parquet/vparquet_reader.cpp:514
11# doris::vectorized::VFileScanner::_get_block_impl(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vfile_scanner.cpp:333
12# doris::vectorized::VScanner::get_block(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vscanner.cpp:132
13# doris::vectorized::VScanner::get_block_after_projects(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vscanner.cpp:99

Because source logical column is the destination logical column if logical converter is consistent. Previously, the reference of column was reset after the conversion was completed, but if an EOF occurred, it was returned in advance, but EOF is not a true error.

if (_logical_converter->is_consistent()) {
            // If logical converter is consistent, _src_logical_column is the final destination column,
            // other components will check the use count
            _src_logical_column.reset();
}

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] 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

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

Copy link
Contributor

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

@AshinGau
Copy link
Member Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -262,9 +280,9 @@ class FixedSizeBinaryConverter : public PhysicalToLogicalConverter {
public:
FixedSizeBinaryConverter(int type_length) : _type_length(type_length) {}

Status physical_convert(ColumnPtr& src_physical_col) override {
Status physical_convert(ColumnPtr& src_physical_col, ColumnPtr& src_logical_column) override {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'physical_convert' can be made const [readability-make-member-function-const]

Suggested change
Status physical_convert(ColumnPtr& src_physical_col, ColumnPtr& src_logical_column) override {
Status physical_convert(ColumnPtr& src_physical_col, ColumnPtr& src_logical_column) const override {

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17597	4518	4453	4453
q2	2001	179	184	179
q3	10452	1162	1224	1162
q4	10180	879	750	750
q5	7490	2669	2567	2567
q6	219	132	129	129
q7	1036	611	579	579
q8	9237	2057	2053	2053
q9	7330	6620	6587	6587
q10	8563	3519	3602	3519
q11	440	239	233	233
q12	455	222	215	215
q13	18302	2968	2966	2966
q14	263	228	232	228
q15	523	479	491	479
q16	509	381	392	381
q17	957	679	756	679
q18	7346	6742	6796	6742
q19	9856	1567	1453	1453
q20	644	320	307	307
q21	3513	2728	2755	2728
q22	367	320	316	316
Total cold run time: 117280 ms
Total hot run time: 38705 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4462	4324	4346	4324
q2	373	268	266	266
q3	3035	2754	2746	2746
q4	1891	1618	1590	1590
q5	5488	5566	5436	5436
q6	218	124	125	124
q7	2325	1975	1993	1975
q8	3264	3390	3421	3390
q9	8795	8802	8716	8716
q10	3939	3742	3753	3742
q11	586	477	479	477
q12	790	611	592	592
q13	17470	3020	2985	2985
q14	300	270	254	254
q15	520	465	469	465
q16	476	410	423	410
q17	1821	1547	1491	1491
q18	8280	7971	7806	7806
q19	2670	1602	1567	1567
q20	2072	1872	1855	1855
q21	12147	4941	4883	4883
q22	561	476	488	476
Total cold run time: 81483 ms
Total hot run time: 55570 ms

morningman
morningman previously approved these changes Apr 21, 2024
@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Apr 21, 2024
Copy link
Contributor

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

Copy link
Contributor

PR approved by anyone and no changes requested.

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Apr 21, 2024
@AshinGau
Copy link
Member Author

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 35.38% (8916/25204)
Line Coverage: 27.10% (73305/270523)
Region Coverage: 26.24% (37880/144354)
Branch Coverage: 23.05% (19290/83684)
Coverage Report: http://coverage.selectdb-in.cc/coverage/5f0c47e25c7751a17ea0b4890f87152ad0ffa482_5f0c47e25c7751a17ea0b4890f87152ad0ffa482/report/index.html

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17598	4439	4449	4439
q2	2000	180	182	180
q3	10468	1118	1182	1118
q4	10222	769	731	731
q5	7529	2635	2627	2627
q6	212	130	132	130
q7	1010	621	570	570
q8	9231	2031	2014	2014
q9	7200	6581	6517	6517
q10	8598	3528	3550	3528
q11	436	232	238	232
q12	461	222	215	215
q13	17782	2925	2977	2925
q14	279	233	230	230
q15	513	487	472	472
q16	524	384	390	384
q17	987	641	677	641
q18	7373	6900	6754	6754
q19	4982	1512	1522	1512
q20	641	317	313	313
q21	3598	2745	2848	2745
q22	366	310	305	305
Total cold run time: 112010 ms
Total hot run time: 38582 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4344	4250	4262	4250
q2	365	265	258	258
q3	2992	2783	2789	2783
q4	1887	1587	1594	1587
q5	5484	5602	5598	5598
q6	217	126	127	126
q7	2380	2019	2020	2019
q8	3276	3387	3385	3385
q9	8777	8833	8816	8816
q10	3969	3705	3727	3705
q11	596	476	485	476
q12	770	610	591	591
q13	17835	3004	2993	2993
q14	300	284	270	270
q15	508	477	470	470
q16	475	415	402	402
q17	1750	1454	1473	1454
q18	8150	7993	7976	7976
q19	1675	1593	1503	1503
q20	2037	1866	1866	1866
q21	11430	4907	4986	4907
q22	560	483	448	448
Total cold run time: 79777 ms
Total hot run time: 55883 ms

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17616	4269	4185	4185
q2	2013	197	191	191
q3	10441	1160	1138	1138
q4	10190	804	728	728
q5	7520	2649	2635	2635
q6	219	129	130	129
q7	1006	645	590	590
q8	9222	2060	2023	2023
q9	7283	6597	6507	6507
q10	8565	3497	3473	3473
q11	447	235	226	226
q12	481	218	224	218
q13	17757	2965	2969	2965
q14	261	230	234	230
q15	520	478	493	478
q16	517	394	380	380
q17	944	705	769	705
q18	7324	6785	6664	6664
q19	4178	1544	1503	1503
q20	670	309	299	299
q21	3514	2707	2907	2707
q22	355	309	307	307
Total cold run time: 111043 ms
Total hot run time: 38281 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4333	4216	4178	4178
q2	356	281	274	274
q3	3000	2706	2792	2706
q4	1858	1628	1565	1565
q5	5251	5279	5302	5279
q6	205	124	120	120
q7	2234	1836	1877	1836
q8	3201	3315	3328	3315
q9	8514	8535	8491	8491
q10	4024	3869	3836	3836
q11	607	490	542	490
q12	849	654	603	603
q13	16440	3344	3215	3215
q14	305	286	277	277
q15	504	476	495	476
q16	524	465	448	448
q17	1807	1548	1498	1498
q18	8107	7902	7857	7857
q19	1732	1571	1578	1571
q20	1978	1805	1859	1805
q21	8355	4968	4942	4942
q22	533	481	448	448
Total cold run time: 74717 ms
Total hot run time: 55230 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 185272 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 5f0c47e25c7751a17ea0b4890f87152ad0ffa482, data reload: false

query1	888	364	353	353
query2	6153	2637	2350	2350
query3	6658	204	196	196
query4	22897	21304	21385	21304
query5	4106	404	413	404
query6	279	193	181	181
query7	4583	286	290	286
query8	234	190	193	190
query9	8386	2330	2303	2303
query10	398	243	257	243
query11	14768	14258	14194	14194
query12	135	87	83	83
query13	1643	364	353	353
query14	9674	7451	7943	7451
query15	273	186	185	185
query16	8201	268	266	266
query17	1924	588	579	579
query18	2102	277	279	277
query19	222	154	155	154
query20	92	86	89	86
query21	201	124	124	124
query22	5069	4912	4786	4786
query23	33831	33152	33274	33152
query24	9067	2962	3079	2962
query25	616	400	408	400
query26	691	159	159	159
query27	2268	365	385	365
query28	5836	2060	2023	2023
query29	873	635	618	618
query30	300	191	173	173
query31	985	767	793	767
query32	93	57	52	52
query33	569	232	233	232
query34	886	507	494	494
query35	810	698	737	698
query36	1085	942	925	925
query37	109	70	71	70
query38	3585	3367	3366	3366
query39	1609	1604	1600	1600
query40	166	133	127	127
query41	46	44	42	42
query42	107	96	93	93
query43	601	527	566	527
query44	1096	744	744	744
query45	295	296	275	275
query46	1142	772	772	772
query47	2061	1943	1959	1943
query48	376	294	296	294
query49	862	396	391	391
query50	788	408	377	377
query51	6796	6826	6723	6723
query52	108	90	89	89
query53	343	277	274	274
query54	260	229	235	229
query55	79	72	74	72
query56	258	227	235	227
query57	1285	1165	1221	1165
query58	218	213	204	204
query59	3629	3598	3150	3150
query60	255	225	232	225
query61	91	80	83	80
query62	585	444	421	421
query63	300	275	269	269
query64	4313	3809	4033	3809
query65	3049	2986	3008	2986
query66	740	327	322	322
query67	15210	15156	15007	15007
query68	8240	536	538	536
query69	584	310	305	305
query70	1246	1125	1137	1125
query71	1457	1261	1263	1261
query72	6609	2645	2442	2442
query73	727	316	321	316
query74	6851	6399	6507	6399
query75	4039	2627	2638	2627
query76	4537	1039	993	993
query77	647	254	254	254
query78	10872	10340	10235	10235
query79	7392	518	521	518
query80	1219	412	418	412
query81	499	238	242	238
query82	804	94	90	90
query83	197	161	157	157
query84	265	82	82	82
query85	970	259	308	259
query86	403	292	303	292
query87	3512	3288	3330	3288
query88	4472	2315	2320	2315
query89	490	362	366	362
query90	1909	178	176	176
query91	124	98	94	94
query92	57	47	46	46
query93	6261	503	500	500
query94	1076	178	174	174
query95	386	286	298	286
query96	595	267	262	262
query97	3136	2915	2915	2915
query98	251	214	217	214
query99	1199	878	858	858
Total cold run time: 288184 ms
Total hot run time: 185272 ms

@doris-robot
Copy link

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

query1	0.04	0.03	0.04
query2	0.07	0.04	0.04
query3	0.23	0.05	0.05
query4	1.69	0.09	0.09
query5	0.50	0.49	0.49
query6	1.46	0.72	0.72
query7	0.02	0.01	0.01
query8	0.05	0.04	0.04
query9	0.54	0.50	0.50
query10	0.53	0.55	0.55
query11	0.15	0.12	0.11
query12	0.15	0.12	0.11
query13	0.59	0.57	0.58
query14	0.75	0.79	0.79
query15	0.82	0.79	0.81
query16	0.36	0.36	0.37
query17	0.99	0.95	1.02
query18	0.22	0.22	0.26
query19	1.76	1.65	1.67
query20	0.02	0.01	0.01
query21	15.40	0.66	0.67
query22	4.63	7.19	1.78
query23	18.27	1.36	1.18
query24	1.40	0.27	0.35
query25	0.15	0.07	0.08
query26	0.26	0.16	0.16
query27	0.07	0.08	0.07
query28	13.35	1.00	0.97
query29	12.62	3.24	3.27
query30	0.26	0.06	0.06
query31	2.86	0.37	0.37
query32	3.30	0.45	0.46
query33	2.78	2.81	2.82
query34	17.06	4.41	4.43
query35	4.50	4.48	4.45
query36	0.65	0.46	0.46
query37	0.18	0.15	0.15
query38	0.15	0.14	0.15
query39	0.04	0.04	0.03
query40	0.17	0.14	0.14
query41	0.09	0.05	0.05
query42	0.06	0.04	0.05
query43	0.04	0.03	0.04
Total cold run time: 109.23 s
Total hot run time: 30.07 s

Copy link
Contributor

@kaka11chen kaka11chen 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 Apr 22, 2024
Copy link
Contributor

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

@morningman morningman merged commit b7b7cad into apache:master Apr 22, 2024
25 of 29 checks passed
yiguolei pushed a commit that referenced this pull request Apr 22, 2024
…lumn (#33932)

Fix error like:
```
8# google::LogMessageFatal::~LogMessageFatal() in /mnt/hdd01/ci/master-deploy/be/lib/doris_be
 9# doris::vectorized::Block::clear_column_data(int) in /mnt/hdd01/ci/master-deploy/be/lib/doris_be
10# doris::vectorized::ParquetReader::get_next_block(doris::vectorized::Block*, unsigned long*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/format/parquet/vparquet_reader.cpp:514
11# doris::vectorized::VFileScanner::_get_block_impl(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vfile_scanner.cpp:333
12# doris::vectorized::VScanner::get_block(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vscanner.cpp:132
13# doris::vectorized::VScanner::get_block_after_projects(doris::RuntimeState*, doris::vectorized::Block*, bool*) at /home/zcp/repo_center/doris_master/doris/be/src/vec/exec/scan/vscanner.cpp:99
```

Because source logical column is the destination logical column if logical converter is consistent. Previously, the reference of column was reset after the conversion was completed, but if an EOF occurred, it was returned in advance, but EOF is not a true error.
```
if (_logical_converter->is_consistent()) {
            // If logical converter is consistent, _src_logical_column is the final destination column,
            // other components will check the use count
            _src_logical_column.reset();
}
```
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. dev/2.1.3-merged reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants