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](replace_column_data)fix replace_column_data semantic #44309

Closed

Conversation

amorynan
Copy link
Contributor

@amorynan amorynan commented Nov 20, 2024

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:
replace_column_data is only for column which is not variable_length column like column_vector...
set is_variable_length = true such as column_string|array|map|struct
should not impl this function, when in agg situation for replace_if_not_null : we should use insert_from!

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?

@amorynan
Copy link
Contributor 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

be/src/vec/columns/column_struct.h Outdated Show resolved Hide resolved
@amorynan
Copy link
Contributor Author

run buildall

Copy link
Contributor

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

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17901	6666	4398	4398
q2	2144	156	149	149
q3	10534	1886	1917	1886
q4	10389	1281	1343	1281
q5	8639	4008	3948	3948
q6	242	125	125	125
q7	2091	1583	1586	1583
q8	9367	2764	2748	2748
q9	10570	9841	9816	9816
q10	8656	3534	3537	3534
q11	426	240	255	240
q12	488	298	309	298
q13	18397	4012	4033	4012
q14	357	324	329	324
q15	524	464	469	464
q16	552	462	456	456
q17	1154	964	927	927
q18	7330	6829	6984	6829
q19	1708	1591	1539	1539
q20	536	319	305	305
q21	4403	4194	4112	4112
q22	494	393	397	393
Total cold run time: 116902 ms
Total hot run time: 49367 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4403	4313	4300	4300
q2	329	230	226	226
q3	4174	4145	4117	4117
q4	2767	2764	2761	2761
q5	7240	7160	7104	7104
q6	237	119	118	118
q7	3297	2900	2826	2826
q8	4383	4504	4529	4504
q9	13749	13824	13632	13632
q10	4270	4280	4328	4280
q11	762	689	686	686
q12	1024	837	855	837
q13	6781	3796	3787	3787
q14	456	417	424	417
q15	513	458	457	457
q16	621	582	579	579
q17	3915	3814	3857	3814
q18	8949	8789	8732	8732
q19	1752	1724	1661	1661
q20	2353	2127	2129	2127
q21	8484	8503	8453	8453
q22	1050	948	908	908
Total cold run time: 81509 ms
Total hot run time: 76326 ms

@doris-robot
Copy link

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

query1	0.02	0.02	0.03
query2	0.07	0.03	0.03
query3	0.25	0.04	0.05
query4	1.78	0.09	0.10
query5	0.53	0.52	0.52
query6	1.22	0.61	0.60
query7	0.02	0.02	0.01
query8	0.03	0.02	0.03
query9	0.53	0.48	0.49
query10	0.54	0.54	0.53
query11	0.13	0.09	0.08
query12	0.11	0.09	0.09
query13	0.62	0.62	0.62
query14	0.79	0.77	0.80
query15	0.78	0.77	0.75
query16	0.36	0.36	0.36
query17	1.01	1.02	0.97
query18	0.19	0.27	0.23
query19	1.93	1.84	1.84
query20	0.02	0.01	0.01
query21	15.45	0.55	0.56
query22	2.05	2.37	1.62
query23	17.26	0.98	0.93
query24	5.91	1.06	1.11
query25	0.34	0.11	0.05
query26	0.65	0.17	0.16
query27	0.04	0.05	0.04
query28	7.19	0.77	0.71
query29	12.66	2.29	2.29
query30	0.62	0.52	0.52
query31	2.80	0.38	0.36
query32	3.35	0.49	0.50
query33	3.08	3.04	3.10
query34	15.28	4.80	4.78
query35	4.84	4.85	4.81
query36	1.06	1.00	1.01
query37	0.06	0.04	0.05
query38	0.03	0.02	0.02
query39	0.02	0.02	0.01
query40	0.15	0.15	0.14
query41	0.07	0.01	0.01
query42	0.02	0.01	0.01
query43	0.02	0.01	0.02
Total cold run time: 103.88 s
Total hot run time: 30.71 s

@doris-robot
Copy link

Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'

Load test result on commit 6cb6a8db91e9edebdf5c75d0ee2ea47c8e5a3b95 with default session variables
Stream load json:         20 seconds loaded 2358488459 Bytes, about 112 MB/s
Stream load orc:          58 seconds loaded 1101869774 Bytes, about 18 MB/s
Stream load parquet:      31 seconds loaded 861443392 Bytes, about 26 MB/s
Insert into select:       21.5 seconds inserted 10000000 Rows, about 465K ops/s

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 38.57% (8312/21550)
Line Coverage: 30.27% (68717/227047)
Region Coverage: 29.69% (35389/119192)
Branch Coverage: 25.45% (18198/71496)
Coverage Report: http://coverage.selectdb-in.cc/coverage/6cb6a8db91e9edebdf5c75d0ee2ea47c8e5a3b95_6cb6a8db91e9edebdf5c75d0ee2ea47c8e5a3b95/report/index.html

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 github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 20, 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.

Copy link
Member

@eldenmoon eldenmoon left a comment

Choose a reason for hiding this comment

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

LGTM

@xiaokang
Copy link
Contributor

do not do it in branch-2.0

@xiaokang xiaokang marked this pull request as draft November 21, 2024 03:11
@amorynan amorynan closed this Dec 4, 2024
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. kind/test reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants