Skip to content

Conversation

@morrySnow
Copy link
Contributor

What problem does this PR solve?

Related PR: #40937

Problem Summary:

We want to provide clearer error messages for window functions with ORDER BY parameters. However, the current error checking incorrectly inspects all child nodes instead of just the immediate/direct child nodes. As a result, when nested window functions are present, this may trigger unexpected error reporting.

errCode = 2, detailMessage = order by is not supported in max(dense_rank() WindowSpec(PARTITION BY lot_no#0 ORDER BY loading_time#20 asc null first)) 

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

@Thearas
Copy link
Contributor

Thearas commented Feb 28, 2025

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?

@morrySnow morrySnow added usercase Important user case type label p0_b dev/2.1.x dev/3.0.x labels Feb 28, 2025
@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 28, 2025
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@morrySnow
Copy link
Contributor Author

run buildall

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17598	5240	5097	5097
q2	2063	316	178	178
q3	10376	1312	739	739
q4	10206	1025	531	531
q5	7542	2406	2369	2369
q6	192	166	132	132
q7	888	758	604	604
q8	9305	1283	1073	1073
q9	5126	4556	4779	4556
q10	6833	2317	1884	1884
q11	504	279	269	269
q12	349	356	219	219
q13	17750	3654	3070	3070
q14	230	236	206	206
q15	510	461	463	461
q16	622	601	585	585
q17	584	867	345	345
q18	6988	6304	6252	6252
q19	1075	947	567	567
q20	321	328	199	199
q21	3043	2227	1963	1963
q22	363	344	319	319
Total cold run time: 102468 ms
Total hot run time: 31618 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5146	5101	5143	5101
q2	243	334	235	235
q3	2203	2689	2323	2323
q4	1443	1830	1343	1343
q5	4202	4127	4160	4127
q6	210	162	121	121
q7	1853	1830	1732	1732
q8	2602	2699	2544	2544
q9	7239	7119	7072	7072
q10	2993	3225	2745	2745
q11	565	496	503	496
q12	696	799	608	608
q13	3532	3842	3279	3279
q14	275	309	286	286
q15	515	461	442	442
q16	653	689	639	639
q17	1145	1605	1327	1327
q18	7483	7349	7436	7349
q19	837	858	971	858
q20	1970	2022	1877	1877
q21	5484	4992	4852	4852
q22	646	599	545	545
Total cold run time: 51935 ms
Total hot run time: 49901 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 186549 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 b3baac61af3a0cf718a4b00430084383700f9bc3, data reload: false

query1	986	395	376	376
query2	6529	1921	1889	1889
query3	6785	212	214	212
query4	27116	23724	23628	23628
query5	4365	744	527	527
query6	313	209	197	197
query7	4595	537	293	293
query8	287	248	241	241
query9	8594	2569	2546	2546
query10	481	327	271	271
query11	16002	15420	15006	15006
query12	165	115	111	111
query13	1652	534	390	390
query14	10437	6810	6919	6810
query15	221	199	192	192
query16	7410	664	465	465
query17	1172	736	563	563
query18	1967	410	300	300
query19	193	194	157	157
query20	125	118	113	113
query21	207	131	106	106
query22	4206	4584	4527	4527
query23	34913	33370	32881	32881
query24	7826	2499	2504	2499
query25	534	482	405	405
query26	1234	285	154	154
query27	2402	506	335	335
query28	4201	2395	2403	2395
query29	696	574	449	449
query30	239	198	165	165
query31	969	861	818	818
query32	72	66	66	66
query33	601	377	319	319
query34	808	905	518	518
query35	851	886	761	761
query36	1013	1028	898	898
query37	124	109	82	82
query38	4306	4144	4208	4144
query39	1491	1433	1401	1401
query40	208	115	109	109
query41	55	53	52	52
query42	125	105	106	105
query43	514	509	492	492
query44	1351	808	795	795
query45	184	181	167	167
query46	938	1149	689	689
query47	1770	1809	1712	1712
query48	419	431	313	313
query49	797	540	431	431
query50	744	812	428	428
query51	4295	4254	4221	4221
query52	111	105	93	93
query53	232	263	188	188
query54	502	490	418	418
query55	82	83	85	83
query56	270	284	304	284
query57	1126	1166	1080	1080
query58	255	240	250	240
query59	2625	2750	2463	2463
query60	286	290	268	268
query61	123	116	117	116
query62	882	807	709	709
query63	238	193	190	190
query64	4085	993	733	733
query65	3351	3269	3257	3257
query66	1109	447	327	327
query67	15727	15112	15498	15112
query68	6203	904	518	518
query69	482	319	278	278
query70	1249	1131	1031	1031
query71	418	312	277	277
query72	5573	3660	3944	3660
query73	751	768	364	364
query74	9313	9135	8952	8952
query75	3350	3420	2801	2801
query76	3251	1328	827	827
query77	500	380	329	329
query78	10086	10353	9440	9440
query79	2267	883	620	620
query80	661	583	462	462
query81	549	296	252	252
query82	220	135	109	109
query83	183	182	162	162
query84	248	98	78	78
query85	753	354	303	303
query86	380	299	277	277
query87	4528	4505	4385	4385
query88	3751	2222	2227	2222
query89	431	354	299	299
query90	1983	199	196	196
query91	132	137	111	111
query92	81	61	56	56
query93	2233	1093	584	584
query94	689	438	308	308
query95	357	274	262	262
query96	504	605	273	273
query97	3355	3459	3295	3295
query98	237	213	210	210
query99	1462	1531	1323	1323
Total cold run time: 274672 ms
Total hot run time: 186549 ms

@doris-robot
Copy link

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

query1	0.04	0.04	0.03
query2	0.08	0.03	0.03
query3	0.24	0.06	0.07
query4	1.60	0.11	0.11
query5	0.55	0.56	0.55
query6	1.19	0.71	0.73
query7	0.03	0.01	0.02
query8	0.04	0.03	0.03
query9	0.59	0.53	0.53
query10	0.56	0.58	0.57
query11	0.16	0.11	0.11
query12	0.16	0.12	0.12
query13	0.61	0.61	0.60
query14	2.69	2.83	2.72
query15	0.92	0.85	0.84
query16	0.39	0.38	0.39
query17	1.02	1.05	1.04
query18	0.22	0.19	0.21
query19	1.96	1.77	2.00
query20	0.01	0.01	0.02
query21	15.35	0.93	0.55
query22	0.76	1.02	0.72
query23	15.03	1.40	0.60
query24	6.87	1.99	1.81
query25	0.54	0.24	0.12
query26	0.59	0.16	0.13
query27	0.05	0.05	0.05
query28	10.33	0.76	0.42
query29	12.61	4.21	3.50
query30	0.25	0.09	0.08
query31	2.80	0.57	0.40
query32	3.24	0.55	0.47
query33	3.08	3.08	3.07
query34	15.78	5.15	4.53
query35	4.53	4.53	4.59
query36	0.67	0.50	0.50
query37	0.09	0.06	0.06
query38	0.05	0.04	0.04
query39	0.03	0.03	0.02
query40	0.18	0.13	0.12
query41	0.09	0.03	0.03
query42	0.03	0.02	0.02
query43	0.04	0.03	0.03
Total cold run time: 106.05 s
Total hot run time: 32.21 s

@yiguolei yiguolei merged commit 4d21c76 into apache:master Mar 5, 2025
30 of 31 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 5, 2025
…48492)

### What problem does this PR solve?

Related PR: #40937

Problem Summary:

We want to provide clearer error messages for window functions with
ORDER BY parameters. However, the current error checking incorrectly
inspects all child nodes instead of just the immediate/direct child
nodes. As a result, when nested window functions are present, this may
trigger unexpected error reporting.

```sql
errCode = 2, detailMessage = order by is not supported in max(dense_rank() WindowSpec(PARTITION BY lot_no#0 ORDER BY loading_time#20 asc null first)) 
```

### Release note

None

### Check List (For Author)

- Test <!-- At least one of them must be included. -->
    - [ ] 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 <!-- Add your reason?  -->

- Behavior changed:
    - [ ] No.
    - [ ] Yes. <!-- Explain the behavior change -->

- Does this need documentation?
    - [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
github-actions bot pushed a commit that referenced this pull request Mar 5, 2025
…48492)

### What problem does this PR solve?

Related PR: #40937

Problem Summary:

We want to provide clearer error messages for window functions with
ORDER BY parameters. However, the current error checking incorrectly
inspects all child nodes instead of just the immediate/direct child
nodes. As a result, when nested window functions are present, this may
trigger unexpected error reporting.

```sql
errCode = 2, detailMessage = order by is not supported in max(dense_rank() WindowSpec(PARTITION BY lot_no#0 ORDER BY loading_time#20 asc null first)) 
```

### Release note

None

### Check List (For Author)

- Test <!-- At least one of them must be included. -->
    - [ ] 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 <!-- Add your reason?  -->

- Behavior changed:
    - [ ] No.
    - [ ] Yes. <!-- Explain the behavior change -->

- Does this need documentation?
    - [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
yiguolei pushed a commit that referenced this pull request Mar 5, 2025
… exception #48492 (#48677)

Cherry-picked from #48492

Co-authored-by: morrySnow <zhangwenxin@selectdb.com>
dataroaring pushed a commit that referenced this pull request Mar 10, 2025
… exception #48492 (#48676)

Cherry-picked from #48492

Co-authored-by: morrySnow <zhangwenxin@selectdb.com>
@gavinchou gavinchou mentioned this pull request Apr 23, 2025
koarz pushed a commit to koarz/doris that referenced this pull request Jun 4, 2025
…pache#48492)

### What problem does this PR solve?

Related PR: apache#40937

Problem Summary:

We want to provide clearer error messages for window functions with
ORDER BY parameters. However, the current error checking incorrectly
inspects all child nodes instead of just the immediate/direct child
nodes. As a result, when nested window functions are present, this may
trigger unexpected error reporting.

```sql
errCode = 2, detailMessage = order by is not supported in max(dense_rank() WindowSpec(PARTITION BY lot_no#0 ORDER BY loading_time#20 asc null first)) 
```

### Release note

None

### Check List (For Author)

- Test <!-- At least one of them must be included. -->
    - [ ] 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 <!-- Add your reason?  -->

- Behavior changed:
    - [ ] No.
    - [ ] Yes. <!-- Explain the behavior change -->

- Does this need documentation?
    - [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
@morrySnow morrySnow deleted the fix_window_order_by branch June 17, 2025 07:13
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.9-merged dev/3.0.5-merged p0_b reviewed usercase Important user case type label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants