Skip to content

[fix](fe) Rewrite LogicalGenerate lateral conjuncts together with generators - #66803

Open
englefly wants to merge 1 commit into
apache:masterfrom
englefly:fd-generate-conjunct
Open

[fix](fe) Rewrite LogicalGenerate lateral conjuncts together with generators#66803
englefly wants to merge 1 commit into
apache:masterfrom
englefly:fd-generate-conjunct

Conversation

@englefly

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:

GenerateExpressionRewrite rewrote only LogicalGenerate.getGenerators() and rebuilt the node via withGenerators(), which preserved the lateral ON conjuncts unchanged. Whole-tree ExprId replacements (e.g. any_value wrapping of a group-by key in EliminateGroupByKeyByUniform / EliminateGroupByKey) that renamed a slot referenced by an ON conjunct therefore left the conjunct with a stale ExprId after the child switched to the wrapped slot, and final slot validation rejected the query:

Generate[UNNEST(tags#T2), ON tag#G = name#N]   // name#N is stale after the child outputs name#N2
  Project[..., name#N2, ...]
    Aggregate[..., ANY_VALUE(name)#N2, ...]

Fix: GenerateExpressionRewrite now rewrites getConjuncts() in the same operation and rebuilds the node with a new LogicalGenerate.withGeneratorsAndConjuncts() helper, so generators and lateral ON conjuncts stay consistent under any expression rewrite.

Release note

None

Check List (For Author)

  • Test

    • Unit Test
      • New GenerateConjunctRewriteTest.testExprIdRewriterRewritesLateralConjuncts: builds a LogicalGenerate with a conjunct referencing a slot, runs ExprIdRewriter with an old->new ExprId map and asserts the conjunct is rewritten (fails on the old code: expected: 999 but was: <old id>).
      • Full GenerateConjunctRewriteTest / EliminateGroupByKeyByUniformTest / MergeGeneratesTest / FdTest classes are green.
    • Regression test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
  • Behavior changed:

    • No.
    • Yes.
      Internal correctness fix for plan rewriting; no intended plan-shape or performance change.
  • 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

…erators

### What problem does this PR solve?

Problem Summary:

GenerateExpressionRewrite rewrote only LogicalGenerate.getGenerators() and
rebuilt the node via withGenerators(), which preserved the lateral ON
conjuncts unchanged. Whole-tree ExprId replacements (e.g. any_value wrapping
of a group-by key in EliminateGroupByKeyByUniform / EliminateGroupByKey) that
renamed a slot referenced by an ON conjunct therefore left the conjunct with a
stale ExprId after the child switched to the wrapped slot, and final slot
validation rejected the query.

Fix: GenerateExpressionRewrite now rewrites getConjuncts() in the same
operation and rebuilds the node with a new
LogicalGenerate.withGeneratorsAndConjuncts() helper, so generators and lateral
ON conjuncts stay consistent under any expression rewrite.

### Release note

None

### Check List (For Author)

- Test: Unit Test
    - New GenerateConjunctRewriteTest.testExprIdRewriterRewritesLateralConjuncts:
      builds a LogicalGenerate with a conjunct referencing a slot, runs
      ExprIdRewriter with an old->new ExprId map and asserts the conjunct is
      rewritten (fails on the old code: expected 999 but was the old id).
    - Full GenerateConjunctRewriteTest / EliminateGroupByKeyByUniformTest /
      MergeGeneratesTest / FdTest classes are green.
- Behavior changed: No. Internal correctness fix for plan rewriting; no
  intended plan-shape or performance change.
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

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?

@englefly

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 84.62% (11/13) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

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

------ Round 1 ----------------------------------
============================================
q1	17553	3159	3126	3126
q2	1875	232	150	150
q3	10488	865	517	517
q4	4670	244	192	192
q5	7684	561	386	386
q6	141	116	93	93
q7	532	496	386	386
q8	9247	957	928	928
q9	3444	2366	2349	2349
q10	6493	828	730	730
q11	438	256	253	253
q12	706	389	325	325
q13	17891	1892	1527	1527
q14	164	156	145	145
q15	q16	441	400	366	366
q17	811	775	822	775
q18	3152	2233	2221	2221
q19	1111	898	800	800
q20	669	551	467	467
q21	5330	1782	1979	1782
q22	328	266	230	230
Total cold run time: 93168 ms
Total hot run time: 17748 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3524	3431	3444	3431
q2	209	215	153	153
q3	2226	2352	2179	2179
q4	1189	1164	885	885
q5	2188	2099	2111	2099
q6	171	123	90	90
q7	1046	925	859	859
q8	1626	1431	1438	1431
q9	3073	3064	3053	3053
q10	1840	1800	1632	1632
q11	358	269	250	250
q12	455	451	342	342
q13	1850	1879	1572	1572
q14	177	167	168	167
q15	q16	398	396	354	354
q17	1044	1034	1026	1026
q18	4966	4376	4744	4376
q19	886	852	867	852
q20	956	949	804	804
q21	3858	3112	3236	3112
q22	389	340	323	323
Total cold run time: 32429 ms
Total hot run time: 28990 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 85219 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 08d8d62d2cefab30711979e4ea69c6d470c696aa, data reload: false

query5	4258	402	322	322
query6	391	171	148	148
query7	4921	454	275	275
query8	282	116	106	106
query9	8673	2887	2879	2879
query10	386	256	219	219
query11	5370	1056	920	920
query12	113	73	70	70
query13	1218	436	336	336
query14	5936	1979	1841	1841
query14_1	1762	1745	1729	1729
query15	172	120	107	107
query16	891	372	339	339
query17	779	428	344	344
query18	2313	332	222	222
query19	155	144	108	108
query20	73	70	71	70
query21	205	113	105	105
query22	5305	5282	5272	5272
query23	7165	6615	6617	6615
query23_1	6535	6532	6489	6489
query24	7286	1037	762	762
query24_1	755	748	770	748
query25	398	297	269	269
query26	1248	277	164	164
query27	2712	420	291	291
query28	4637	1524	1524	1524
query29	950	457	369	369
query30	269	191	152	152
query31	972	669	621	621
query32	103	51	50	50
query33	458	215	177	177
query34	974	836	470	470
query35	402	395	356	356
query36	557	550	533	533
query37	125	90	72	72
query38	993	847	825	825
query39	533	543	539	539
query39_1	487	517	514	514
query40	223	124	115	115
query41	58	55	55	55
query42	76	74	74	74
query43	246	252	216	216
query44	1041	567	572	567
query45	117	109	100	100
query46	813	867	553	553
query47	978	990	954	954
query48	315	314	245	245
query49	537	244	196	196
query50	840	325	259	259
query51	7943	8063	7959	7959
query52	77	70	71	70
query53	209	214	153	153
query54	219	180	181	180
query55	67	57	53	53
query56	218	224	263	224
query57	652	616	598	598
query58	212	198	200	198
query59	1104	1082	964	964
query60	259	205	222	205
query61	134	127	124	124
query62	372	200	177	177
query63	188	168	159	159
query64	2729	678	592	592
query65	1556	1539	1541	1539
query66	1792	294	242	242
query67	9543	9754	9659	9659
query68	2761	1224	795	795
query69	333	220	209	209
query70	626	577	580	577
query71	282	238	244	238
query72	2284	1721	1539	1539
query73	653	605	322	322
query74	1575	1222	1145	1145
query75	1212	1134	999	999
query76	2289	729	573	573
query77	248	256	193	193
query78	5042	4700	4442	4442
query79	1176	802	577	577
query80	1169	403	329	329
query81	454	194	177	177
query82	584	143	112	112
query83	317	252	236	236
query84	301	124	99	99
query85	838	439	372	372
query86	386	169	172	169
query87	1010	969	900	900
query88	2788	2127	2135	2127
query89	306	238	204	204
query90	1837	147	142	142
query91	148	142	122	122
query92	50	45	42	42
query93	1205	1158	774	774
query94	613	267	214	214
query95	649	424	350	350
query96	820	589	293	293
query97	1065	1042	991	991
query98	141	135	127	127
query99	411	354	309	309
Total cold run time: 176283 ms
Total hot run time: 85219 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.5 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 08d8d62d2cefab30711979e4ea69c6d470c696aa, data reload: false

query1	0.01	0.01	0.00
query2	0.07	0.04	0.03
query3	0.24	0.12	0.11
query4	1.60	0.10	0.10
query5	0.16	0.15	0.16
query6	1.26	0.68	0.67
query7	0.03	0.01	0.01
query8	0.05	0.03	0.03
query9	0.30	0.21	0.21
query10	0.37	0.34	0.32
query11	0.17	0.11	0.11
query12	0.15	0.12	0.12
query13	0.30	0.30	0.32
query14	0.44	0.46	0.45
query15	0.36	0.33	0.35
query16	0.21	0.23	0.21
query17	0.66	0.66	0.64
query18	0.19	0.17	0.16
query19	1.18	1.13	1.10
query20	0.01	0.01	0.01
query21	15.43	0.16	0.11
query22	5.05	0.04	0.04
query23	16.18	0.26	0.10
query24	3.00	0.30	0.28
query25	0.10	0.04	0.04
query26	0.88	0.18	0.12
query27	0.04	0.02	0.02
query28	3.66	0.57	0.26
query29	12.47	3.16	2.59
query30	0.25	0.10	0.12
query31	2.75	0.37	0.17
query32	3.54	0.32	0.22
query33	1.35	1.55	1.37
query34	15.37	2.21	1.78
query35	1.72	1.74	1.73
query36	0.46	0.28	0.28
query37	0.07	0.04	0.04
query38	0.05	0.03	0.03
query39	0.03	0.02	0.02
query40	0.11	0.08	0.07
query41	0.08	0.03	0.02
query42	0.03	0.02	0.02
query43	0.03	0.03	0.03
Total cold run time: 90.41 s
Total hot run time: 14.5 s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants