You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "yujun777 (via GitHub)" <gi...@apache.org> on 2026/04/23 06:45:40 UTC
[PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
yujun777 opened a new pull request, #62742:
URL: https://github.com/apache/doris/pull/62742
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #62705
Problem Summary:
Following up on #62705, several Nereids rewrite rules still moved predicates
containing non-idempotent (unique) functions such as rand() / uuid() /
random_bytes() across operator boundaries in ways that changed query
semantics. The common root cause is that a predicate like `rand() > 0.5`
has an empty input-slot set, so the `containsAll(emptySet)` / `allMatch`
guards used by these rules silently returned true and allowed the push
down / elimination.
This PR adds `containsUniqueFunction()` guards to the following rules:
1. PushDownFilterThroughRepeat: skip conjuncts with unique functions.
Pushing `rand() > x` below Repeat changes which rows feed each
grouping set and alters aggregate results.
2. PushDownFilterThroughWindow: skip conjuncts with unique functions.
Pushing a unique predicate below a window operator re-samples the
base rows and changes every window-function value.
3. PushDownFilterThroughPartitionTopN: same as Window — skip unique
conjuncts in the split loop.
4. PushDownFilterThroughSort (sort-elimination branch): do not drop the
sort when any order key contains a unique function. `ORDER BY rand()`
has empty input slots, so the old `allMatch` on an empty stream
returned true and wrongly eliminated the sort.
5. PushDownTopNThroughUnion / PushDownTopNDistinctThroughUnion: do not
push a TopN whose order keys contain unique functions below Union.
With non-idempotent ordering each branch would be sorted by a
different random draw than the global one.
### Release note
Fix wrong results when predicates / order keys containing rand(), uuid(),
random_bytes() or uuid_numeric() were pushed across Repeat, Window,
PartitionTopN, Sort, or Union boundaries.
### Check List (For Author)
- Test:
- Unit Test (FE): PushDownFilterThroughRepeatTest (3 new tests),
PushDownFilterThroughSortTest (+1 test for unique order key),
PushDownFilterThroughWindowTest (+1 test for unique conjunct).
- Regression Test: 5 new suites under
regression-test/suites/nereids_rules_p0/unique_function/.
- Behavior changed: Yes (queries that previously returned incorrect
results due to unsafe pushdown of unique-function predicates / order
keys will now return correct results).
- Does this need documentation: No
Co-authored-by: Copilot <22...@users.noreply.github.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4302850841
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3130364619
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -120,11 +120,14 @@ public Rule build() {
Set<Expression> rightPredicates = Sets.newLinkedHashSet();
Set<Expression> remainingPredicates = Sets.newLinkedHashSet();
for (Expression p : filterPredicates) {
- if (p.containsUniqueFunction()) {
+ Set<Slot> slots = p.collect(SlotReference.class::isInstance);
+ if (p.containsUniqueFunction() && slots.isEmpty()) {
Review Comment:
This new `&& slots.isEmpty()` guard is still too narrow. A one-sided predicate like `WHERE t2.id + rand() > 0.2` above a `LogicalJoin` is evaluated once per joined row in the original plan, but after pushing it into the right child it is evaluated once per `t2` row and then reused for every matching left row. With one right row joining two left rows, the original plan can keep one pair and drop the other, while the rewritten plan can only keep both or none.
That makes this a wrong-result path distinct from the already-discussed `PushDownJoinOtherCondition` thread, and the updated `push_down_filter_through_join_3/4` regression outputs now lock in that unsafe plan.
```suggestion
if (p.containsUniqueFunction()) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3332794834
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -120,11 +120,14 @@ public Rule build() {
Set<Expression> rightPredicates = Sets.newLinkedHashSet();
Set<Expression> remainingPredicates = Sets.newLinkedHashSet();
for (Expression p : filterPredicates) {
- if (p.containsUniqueFunction()) {
+ Set<Slot> slots = p.collect(SlotReference.class::isInstance);
+ if (p.containsUniqueFunction() && slots.isEmpty()) {
Review Comment:
This is addressed after the rebase to current upstream/master. `PushDownFilterThroughJoin` now checks `p.containsVolatileExpression()` before slot-based pushdown, so both empty-slot predicates like `rand() > 0.1` and one-sided predicates like `t2.id + rand() > 0.2` are kept in `remainingPredicates` above the join. I also updated the regression case comment to reflect that volatile predicates should stay above the join even when they reference only one side.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -120,11 +120,14 @@ public Rule build() {
Set<Expression> rightPredicates = Sets.newLinkedHashSet();
Set<Expression> remainingPredicates = Sets.newLinkedHashSet();
for (Expression p : filterPredicates) {
- if (p.containsUniqueFunction()) {
+ Set<Slot> slots = p.collect(SlotReference.class::isInstance);
+ if (p.containsUniqueFunction() && slots.isEmpty()) {
Review Comment:
This has been fixed by the current rebased code as well. The old narrowed guard no longer exists on this branch: `PushDownFilterThroughJoin` first routes any `containsVolatileExpression()` predicate to `remainingPredicates`, before checking input slots. Therefore a one-sided volatile predicate such as `WHERE t2.id + rand() > 0.2` is not pushed into the right child and remains evaluated per joined row. I updated the test comment accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4603801938
<details>
<summary>TPC-DS: <b>Total hot run time: 169256 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 10ca6e7edcbcd5d69aebf8b4bdd4b7c7cd667975, data reload: false
query5 4336 631 489 489
query6 441 201 183 183
query7 4880 559 319 319
query8 391 223 206 206
query9 8770 4006 3979 3979
query10 448 312 270 270
query11 5932 2382 2125 2125
query12 160 101 97 97
query13 1263 601 421 421
query14 6336 5349 5066 5066
query14_1 4387 4371 4389 4371
query15 206 199 174 174
query16 987 448 420 420
query17 1081 688 553 553
query18 2471 480 336 336
query19 212 182 139 139
query20 142 109 113 109
query21 234 136 120 120
query22 13620 13502 13464 13464
query23 17364 16464 16199 16199
query23_1 16207 16231 16231 16231
query24 7628 1776 1303 1303
query24_1 1273 1310 1304 1304
query25 544 453 369 369
query26 1300 322 167 167
query27 2701 584 341 341
query28 4443 1986 1985 1985
query29 1110 607 472 472
query30 310 236 196 196
query31 1121 1077 962 962
query32 122 62 59 59
query33 533 341 262 262
query34 1191 1138 654 654
query35 756 791 686 686
query36 1433 1402 1261 1261
query37 165 110 96 96
query38 3235 3146 3038 3038
query39 939 919 906 906
query39_1 876 879 883 879
query40 247 132 109 109
query41 72 70 70 70
query42 95 100 99 99
query43 318 325 281 281
query44
query45 197 188 188 188
query46 1118 1247 762 762
query47 2401 2364 2336 2336
query48 416 407 308 308
query49 663 489 373 373
query50 1017 372 261 261
query51 4370 4293 4274 4274
query52 91 92 78 78
query53 250 278 196 196
query54 301 232 218 218
query55 82 79 73 73
query56 271 243 226 226
query57 1447 1443 1340 1340
query58 273 235 223 223
query59 1623 1671 1479 1479
query60 301 261 245 245
query61 196 193 181 181
query62 714 665 590 590
query63 235 188 187 187
query64 2647 875 708 708
query65
query66 1844 494 370 370
query67 29690 29595 29384 29384
query68
query69 452 316 274 274
query70 1006 1001 1020 1001
query71 304 213 215 213
query72 2937 2639 2388 2388
query73 864 753 423 423
query74 5207 4995 4768 4768
query75 2686 2583 2225 2225
query76 2313 1144 765 765
query77 354 386 275 275
query78 12321 12455 11830 11830
query79 1377 1071 783 783
query80 591 470 385 385
query81 448 292 244 244
query82 854 152 125 125
query83 364 277 250 250
query84 278 141 111 111
query85 881 554 426 426
query86 368 321 302 302
query87 3358 3362 3204 3204
query88 3630 2753 2708 2708
query89 423 375 330 330
query90 1954 183 190 183
query91 178 170 136 136
query92 63 63 59 59
query93 1514 1446 946 946
query94 521 378 320 320
query95 673 469 345 345
query96 1106 811 351 351
query97 2726 2709 2540 2540
query98 209 206 201 201
query99 1188 1179 1044 1044
Total cold run time: 251985 ms
Total hot run time: 169256 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4591870200
# FE UT Coverage Report
Increment line coverage ` 71.15% (37/52)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_141e4bfc520388c1f72728f0004eab77e5fd35f0/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_141e4bfc520388c1f72728f0004eab77e5fd35f0/fe_report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3339024337
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOtherCondition.java:
##########
@@ -75,7 +75,16 @@ public Rule build() {
Set<Expression> rightConjuncts = Sets.newHashSet();
for (Expression otherConjunct : otherJoinConjuncts) {
- if (PUSH_DOWN_LEFT_VALID_TYPE.contains(join.getJoinType())
+ // Keep slot-free volatile predicates in otherJoinConjuncts. Otherwise
+ // `allCoveredBy(..., child.getOutputSet())` would arbitrarily push
+ // `rand() > 0.5` into one side because empty slots are covered by both
+ // children. Side-specific volatile predicates are still pushed down for
+ // performance, matching PostgreSQL's treatment of single-relation INNER
+ // JOIN ON quals as base restrictions.
+ if (otherConjunct.containsVolatileExpression()
Review Comment:
This guard is still too narrow for volatile ON predicates. A one-sided condition such as `t1.id + rand() > 0.5` has non-empty input slots, so it falls through to `allCoveredBy(...)` and is pushed into one child. For an INNER/CROSS JOIN, though, the ON predicate is evaluated per joined pair; after this rewrite the random value is evaluated once per input row and reused for every matching row from the other side. With one left row joining two right rows, the original plan can keep one pair and drop the other, while the pushed plan can only keep both or none. `PushDownFilterThroughJoin` already keeps all `containsVolatileExpression()` predicates above the join for this reason, so this path should not push side-specific volatile ON conjuncts either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow merged PR #62742:
URL: https://github.com/apache/doris/pull/62742
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622750551
<details>
<summary>TPC-H: <b>Total hot run time: 29278 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 4639cadc23aed6d31474a1b45c463a6b10093534, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17596 4049 4014 4014
q2 q3 10739 1378 843 843
q4 4690 492 343 343
q5 7549 871 578 578
q6 186 169 151 151
q7 774 828 642 642
q8 9882 1697 1596 1596
q9 6831 4437 4430 4430
q10 6803 1797 1528 1528
q11 427 266 244 244
q12 645 418 299 299
q13 18127 3398 2780 2780
q14 267 254 256 254
q15 q16 826 766 707 707
q17 903 964 1006 964
q18 6751 5815 5598 5598
q19 1314 1283 1114 1114
q20 513 406 261 261
q21 6174 2796 2617 2617
q22 444 374 315 315
Total cold run time: 101441 ms
Total hot run time: 29278 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 4725 4717 4744 4717
q2 q3 5058 5152 4598 4598
q4 2099 2173 1407 1407
q5 4840 4797 4638 4638
q6 237 176 127 127
q7 1830 1729 1640 1640
q8 2367 2066 1958 1958
q9 7398 7418 7369 7369
q10 4745 4703 4228 4228
q11 538 384 353 353
q12 740 735 560 560
q13 3043 3365 2798 2798
q14 284 279 267 267
q15 q16 679 697 605 605
q17 1281 1258 1252 1252
q18 7278 6828 6739 6739
q19 1097 1095 1096 1095
q20 2217 2195 1967 1967
q21 5322 4602 4428 4428
q22 535 453 401 401
Total cold run time: 56313 ms
Total hot run time: 51147 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4603124708
<details>
<summary>TPC-H: <b>Total hot run time: 28792 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 0e523879874e8209bb77e968800affbbade23fc5, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17814 4132 4198 4132
q2 q3 10772 1414 825 825
q4 4686 472 349 349
q5 7510 866 603 603
q6 196 179 142 142
q7 790 844 617 617
q8 10038 1538 1620 1538
q9 7059 4502 4500 4500
q10 6846 1792 1508 1508
q11 445 274 250 250
q12 629 425 299 299
q13 18111 3449 2778 2778
q14 272 264 242 242
q15 q16 817 780 708 708
q17 1702 1142 820 820
q18 6856 5897 5455 5455
q19 1382 1231 983 983
q20 509 410 266 266
q21 5611 2616 2475 2475
q22 426 354 302 302
Total cold run time: 102471 ms
Total hot run time: 28792 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 4386 4280 4423 4280
q2 q3 4506 4950 4340 4340
q4 2122 2224 1392 1392
q5 4509 4340 4337 4337
q6 232 175 130 130
q7 2078 1922 1649 1649
q8 2648 2274 2210 2210
q9 8009 8011 8052 8011
q10 4801 4747 4387 4387
q11 574 423 383 383
q12 742 758 544 544
q13 3281 3560 2967 2967
q14 296 338 292 292
q15 q16 734 744 636 636
q17 1409 1337 1351 1337
q18 8048 7515 6766 6766
q19 1144 1089 1118 1089
q20 2235 2222 1933 1933
q21 5326 4601 4489 4489
q22 520 466 399 399
Total cold run time: 57600 ms
Total hot run time: 51571 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4597847803
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3130066368
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughRepeat.java:
##########
@@ -72,7 +72,8 @@ public Rule build() {
Set<Expression> notPushedPredicates = Sets.newHashSet();
for (Expression conjunct : filter.getConjuncts()) {
Set<Slot> conjunctSlots = conjunct.getInputSlots();
- if (commonGroupingSetExpressions.containsAll(conjunctSlots)) {
+ if (commonGroupingSetExpressions.containsAll(conjunctSlots)
+ && !conjunct.containsUniqueFunction()) {
Review Comment:
See main PR comment — the three raised concerns (`PushDownJoinOtherCondition`, `PushDownFilterThroughSetOperation`, normalized-slot NLJ path) are respectively already fixed in this PR, already fixed in the follow-up #62754, and a pre-existing `BETWEEN`-expansion trade-off out of scope for this PR.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNThroughUnion.java:
##########
@@ -61,6 +61,8 @@ public class PushDownTopNThroughUnion implements RewriteRuleFactory {
public List<Rule> buildRules() {
return ImmutableList.of(
logicalTopN(logicalUnion().when(union -> union.getQualifier() == Qualifier.ALL))
+ .when(topN -> topN.getOrderKeys().stream()
+ .noneMatch(key -> key.getExpr().containsUniqueFunction()))
Review Comment:
See main PR comment — the three raised concerns (`PushDownJoinOtherCondition`, `PushDownFilterThroughSetOperation`, normalized-slot NLJ path) are respectively already fixed in this PR, already fixed in the follow-up #62754, and a pre-existing `BETWEEN`-expansion trade-off out of scope for this PR.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOtherCondition.java:
##########
@@ -75,7 +75,16 @@ public Rule build() {
Set<Expression> rightConjuncts = Sets.newHashSet();
for (Expression otherConjunct : otherJoinConjuncts) {
- if (PUSH_DOWN_LEFT_VALID_TYPE.contains(join.getJoinType())
+ // A conjunct that contains a unique (non-idempotent) function such as
+ // rand()/uuid() must NOT be pushed into a single child. Evaluating it
+ // per-input-row instead of per-join-pair changes semantics (every
+ // surviving row has the filter re-drawn once instead of once per pair).
+ // In addition, a predicate like `rand() > 0.5` has empty input slots,
+ // so `allCoveredBy(..., child.getOutputSet())` would otherwise wrongly
+ // return true because `Set.containsAll(emptySet) == true`.
+ if (otherConjunct.containsUniqueFunction()) {
Review Comment:
See main PR comment — the three raised concerns (`PushDownJoinOtherCondition`, `PushDownFilterThroughSetOperation`, normalized-slot NLJ path) are respectively already fixed in this PR, already fixed in the follow-up #62754, and a pre-existing `BETWEEN`-expansion trade-off out of scope for this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4303866923
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4302262817
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3360854172
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> volatileInputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(volatileInputSlots.isEmpty() ? target.getInputSlots() : volatileInputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
+ }
+ Set<Slot> inputSlots = volatileExpressionSlots.get(entry.getKey());
+ Set<Slot> volatileInputSlots = entry.getKey().getInputSlots();
+ if (!volatileInputSlots.isEmpty()
+ && !leftOutputSet.containsAll(inputSlots)
+ && !rightOutputSet.containsAll(inputSlots)) {
+ continue;
+ }
+ ExprId exprId = StatementScopeIdGenerator.newExprId();
+ String functionName = entry.getKey() instanceof Function
+ ? ((Function) entry.getKey()).getName() : "volatile";
+ Alias alias = new Alias(exprId, entry.getKey(), "$_" + functionName + "_" + exprId.asInt() + "_$");
+ replaceMap.put(alias.child(0), alias.toSlot());
+ // Join can not add a project at join-pair scope, but repeated volatile expressions
+ // still need one materialized value. Slot-free volatile functions use the containing
+ // conjunct's slots to choose a side, so t2.k + rand() can project rand() on the right.
+ // Volatile functions with input slots use their own slots to avoid projecting
+ // volatile_udf(t2.k) on the left only because its containing conjunct also uses t1.
+ // Volatile functions whose own slots span both join children cannot be projected into
+ // either child, so they are not rewritten here.
+ // Put right-only expressions on the right child; otherwise keep the previous
+ // left-child behavior as the conservative default.
+ if (!inputSlots.isEmpty() && rightOutputSet.containsAll(inputSlots)) {
+ rightAliases.add(alias);
+ } else {
Review Comment:
This right-side materialization is still unsafe for side-specific volatile ON predicates, not only for the mixed-slot case discussed above. For example, `t1 join t2 on t1.k = t2.k and t2.v + rand() between 0.1 and 0.5`: if two `t1` rows match the same `t2` row, the original ON predicate evaluates `rand()` once per joined pair, so one pair can pass while the other fails. After this rewrite, `rand()` is evaluated once in the right child project/filter and then reused for every matching left row, so the plan can only keep both pairs or drop both. That is the same granularity change that `PushDownJoinOtherCondition` now avoids for direct volatile ON conjuncts. If we cannot materialize at join-pair scope, this join rewrite should skip repeated volatile ON expressions instead of moving them into either child; the `join_other_unique_6` expected plan currently locks in the wrong behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3361257768
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> volatileInputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(volatileInputSlots.isEmpty() ? target.getInputSlots() : volatileInputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
+ }
+ Set<Slot> inputSlots = volatileExpressionSlots.get(entry.getKey());
+ Set<Slot> volatileInputSlots = entry.getKey().getInputSlots();
+ if (!volatileInputSlots.isEmpty()
+ && !leftOutputSet.containsAll(inputSlots)
+ && !rightOutputSet.containsAll(inputSlots)) {
+ continue;
+ }
+ ExprId exprId = StatementScopeIdGenerator.newExprId();
+ String functionName = entry.getKey() instanceof Function
+ ? ((Function) entry.getKey()).getName() : "volatile";
+ Alias alias = new Alias(exprId, entry.getKey(), "$_" + functionName + "_" + exprId.asInt() + "_$");
+ replaceMap.put(alias.child(0), alias.toSlot());
+ // Join can not add a project at join-pair scope, but repeated volatile expressions
+ // still need one materialized value. Slot-free volatile functions use the containing
+ // conjunct's slots to choose a side, so t2.k + rand() can project rand() on the right.
+ // Volatile functions with input slots use their own slots to avoid projecting
+ // volatile_udf(t2.k) on the left only because its containing conjunct also uses t1.
+ // Volatile functions whose own slots span both join children cannot be projected into
+ // either child, so they are not rewritten here.
+ // Put right-only expressions on the right child; otherwise keep the previous
+ // left-child behavior as the conservative default.
+ if (!inputSlots.isEmpty() && rightOutputSet.containsAll(inputSlots)) {
+ rightAliases.add(alias);
+ } else {
Review Comment:
One possible refinement is to handle inner/cross join specially: move the volatile ON predicate to a Filter above the join, then let AddProjectForVolatileExpression materialize the volatile expression at join-output scope. That would be more accurate for inner/cross join.
But this still does not solve outer/semi/anti/mark join, where moving ON predicates above the join is not generally equivalent. So we would end up with different volatile-expression semantics for different join types, while the original problem of duplicated volatile evaluation still exists for the remaining join types.
Considering this tradeoff, I think it is better to keep the current implementation simple and consistent for now. The current approach is not perfect for all join semantics, but it avoids the more serious duplicated-evaluation problem such as expanded BETWEEN predicates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4592073680
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4303867086
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4610454277
<details>
<summary>TPC-H: <b>Total hot run time: 29380 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 495775c875ff389976dcf7fec551953b47d5665a, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17697 4059 4031 4031
q2 q3 10804 1458 841 841
q4 4681 477 340 340
q5 7546 901 583 583
q6 184 176 140 140
q7 764 840 625 625
q8 9365 1374 1574 1374
q9 6040 4525 4553 4525
q10 6807 1810 1542 1542
q11 434 279 249 249
q12 626 424 290 290
q13 18141 3593 2826 2826
q14 268 255 238 238
q15 q16 814 780 722 722
q17 967 863 909 863
q18 7032 5864 5727 5727
q19 1307 1303 1078 1078
q20 521 402 259 259
q21 6146 2851 2805 2805
q22 467 377 322 322
Total cold run time: 100611 ms
Total hot run time: 29380 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 5085 4913 4762 4762
q2 q3 4911 5336 4660 4660
q4 2147 2190 1390 1390
q5 4842 4937 4717 4717
q6 237 175 127 127
q7 1859 1809 1557 1557
q8 2443 2151 2129 2129
q9 7919 7535 7475 7475
q10 4752 4699 4200 4200
q11 541 383 358 358
q12 728 833 525 525
q13 3042 3468 2833 2833
q14 278 277 254 254
q15 q16 694 695 621 621
q17 1284 1248 1258 1248
q18 7227 6685 6637 6637
q19 1145 1131 1108 1108
q20 2219 2220 1944 1944
q21 5317 4704 4429 4429
q22 508 448 414 414
Total cold run time: 57178 ms
Total hot run time: 51388 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "Thearas (via GitHub)" <gi...@apache.org>.
Thearas commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4302262878
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4608664796
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3354660430
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
This guard still lets an empty-slot volatile expression be projected into one child when the containing join conjunct references both sides. For example, after BETWEEN expansion, `t1.id + rand() between t2.id and t2.id + 1` contributes the same `rand()` node twice; `volatileInputSlots` is empty, while `inputSlots` collected from the containing targets includes both left and right slots. The current guard does not skip that case, then the side selection below falls through to `leftAliases`, so `rand()` is evaluated once per left row and reused for every matching right row. The original ON predicate evaluates it per joined pair, so with one left row matching two right rows the original can keep one pair and reject the other, while the rewritten plan can only keep both or neither. Please also skip slot-free volatile expressions when the collected containing `inputSlots` are not fully covered by either child, or otherwise keep them in the join without child projection. This is distinc
t from the existing direct ON-pushdown comments because it is introduced by the new repeated-volatile materialization path after the volatile expression has been selected for aliasing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3356110310
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
I think this is the less-bad behavior under Doris's current join/project split. A cannot represent a per-pair project for a volatile expression, so if the same volatile node appears multiple times after rewrites like expansion, leaving it in the join means it will be re-evaluated independently for each occurrence and can diverge within the same predicate. By materializing it into one child project, we at least force repeated references to share a single value, which preserves the one-value-per-expression property that the original predicate needs. This is not fully equivalent to the join-pair semantics, but it is strictly better than keeping the duplicated volatile expression on the join and letting it be re-sampled separately.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
I think this is the less-bad behavior under Doris's current join/project split. A LogicalJoin cannot represent a per-pair project for a volatile expression, so if the same volatile node appears multiple times after rewrites like BETWEEN expansion, leaving it in the join means it will be re-evaluated independently for each occurrence and can diverge within the same predicate. By materializing it into one child project, we at least force repeated references to share a single value, which preserves the one-value-per-expression property that the original predicate needs. This is not fully equivalent to the join-pair semantics, but it is strictly better than keeping the duplicated volatile expression on the join and letting it be re-sampled separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622369729
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4592937712
<details>
<summary>TPC-H: <b>Total hot run time: 29246 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 141e4bfc520388c1f72728f0004eab77e5fd35f0, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17609 4126 4055 4055
q2 q3 10815 1379 804 804
q4 4685 476 341 341
q5 7554 890 594 594
q6 183 175 137 137
q7 773 863 632 632
q8 9434 1593 1592 1592
q9 5870 4479 4472 4472
q10 6829 1812 1535 1535
q11 430 274 251 251
q12 636 424 291 291
q13 18160 3370 2773 2773
q14 270 252 236 236
q15 q16 815 775 705 705
q17 982 885 956 885
q18 7037 5693 5660 5660
q19 1323 1209 1142 1142
q20 521 409 265 265
q21 6246 2772 2562 2562
q22 490 395 314 314
Total cold run time: 100662 ms
Total hot run time: 29246 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 5263 5053 5005 5005
q2 q3 4864 5250 4683 4683
q4 2091 2224 1387 1387
q5 4815 4779 4913 4779
q6 234 179 136 136
q7 1870 1751 1586 1586
q8 2400 2129 2105 2105
q9 8251 7938 7492 7492
q10 4735 4674 4250 4250
q11 524 384 353 353
q12 727 737 525 525
q13 2971 3410 2744 2744
q14 261 278 254 254
q15 q16 677 697 612 612
q17 1277 1253 1253 1253
q18 7168 6671 6733 6671
q19 1122 1088 1129 1088
q20 2205 2218 1935 1935
q21 5239 4979 4429 4429
q22 524 450 424 424
Total cold run time: 57218 ms
Total hot run time: 51711 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4598222448
# FE UT Coverage Report
Increment line coverage ` 71.15% (37/52)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_ece53853ea7620ca5434d974848d02e9ab411db8/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_ece53853ea7620ca5434d974848d02e9ab411db8/fe_report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4610535768
<details>
<summary>TPC-DS: <b>Total hot run time: 169330 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 495775c875ff389976dcf7fec551953b47d5665a, data reload: false
query5 4321 626 470 470
query6 441 198 184 184
query7 4827 559 329 329
query8 376 211 213 211
query9 8820 4082 4031 4031
query10 472 308 258 258
query11 5952 2327 2190 2190
query12 163 104 99 99
query13 1245 627 443 443
query14 6376 5390 5069 5069
query14_1 4406 4434 4349 4349
query15 203 193 176 176
query16 1032 446 424 424
query17 1096 701 572 572
query18 2512 471 335 335
query19 198 187 137 137
query20 110 107 107 107
query21 221 140 120 120
query22 13876 13594 13405 13405
query23 17350 16506 16178 16178
query23_1 16298 16312 16249 16249
query24 7452 1753 1307 1307
query24_1 1315 1348 1320 1320
query25 539 447 387 387
query26 1319 321 165 165
query27 2684 570 346 346
query28 4476 2027 2030 2027
query29 1083 618 508 508
query30 320 230 197 197
query31 1132 1073 963 963
query32 116 59 59 59
query33 527 312 249 249
query34 1199 1192 629 629
query35 777 792 671 671
query36 1373 1402 1224 1224
query37 155 104 95 95
query38 3235 3135 3059 3059
query39 932 911 913 911
query39_1 874 876 877 876
query40 240 120 114 114
query41 65 64 62 62
query42 97 92 96 92
query43 321 320 280 280
query44
query45 192 186 186 186
query46 1120 1175 737 737
query47 2363 2399 2237 2237
query48 379 406 296 296
query49 640 452 355 355
query50 1035 362 258 258
query51 4336 4255 4269 4255
query52 86 86 76 76
query53 238 265 192 192
query54 268 232 195 195
query55 77 75 72 72
query56 228 220 225 220
query57 1434 1416 1308 1308
query58 243 214 209 209
query59 1567 1641 1437 1437
query60 283 246 237 237
query61 161 156 152 152
query62 693 671 585 585
query63 239 189 186 186
query64 2610 851 687 687
query65
query66 1815 480 369 369
query67 29755 29752 29568 29568
query68
query69 449 314 278 278
query70 998 947 954 947
query71 300 233 215 215
query72 3210 2939 2479 2479
query73 835 772 422 422
query74 5203 4958 4758 4758
query75 2668 2599 2261 2261
query76 2297 1136 774 774
query77 363 381 281 281
query78 12605 12526 11855 11855
query79 1308 1113 792 792
query80 578 475 381 381
query81 457 286 241 241
query82 560 156 126 126
query83 350 276 247 247
query84 266 149 117 117
query85 864 535 440 440
query86 371 302 277 277
query87 3370 3347 3200 3200
query88 3615 2756 2725 2725
query89 428 378 339 339
query90 1954 178 181 178
query91 178 176 137 137
query92 64 61 56 56
query93 1469 1584 908 908
query94 550 369 350 350
query95 672 389 344 344
query96 1046 793 365 365
query97 2702 2732 2568 2568
query98 213 204 207 204
query99 1166 1164 1027 1027
Total cold run time: 251896 ms
Total hot run time: 169330 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4598692180
# FE Regression Coverage Report
Increment line coverage ` 33.78% (50/148)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_ece53853ea7620ca5434d974848d02e9ab411db8_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_ece53853ea7620ca5434d974848d02e9ab411db8_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4593023888
<details>
<summary>TPC-DS: <b>Total hot run time: 170126 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 141e4bfc520388c1f72728f0004eab77e5fd35f0, data reload: false
query5 4329 641 522 522
query6 329 226 199 199
query7 4222 589 314 314
query8 343 239 242 239
query9 8818 4001 4013 4001
query10 466 337 303 303
query11 5774 2338 2218 2218
query12 182 138 130 130
query13 1352 617 461 461
query14 6144 5424 5152 5152
query14_1 4468 4447 4430 4430
query15 214 208 186 186
query16 1018 460 487 460
query17 1157 761 628 628
query18 2731 511 376 376
query19 250 210 167 167
query20 143 135 134 134
query21 219 140 124 124
query22 13641 13588 13573 13573
query23 17401 16683 16174 16174
query23_1 16393 16379 16396 16379
query24 7400 1780 1314 1314
query24_1 1269 1312 1331 1312
query25 584 514 457 457
query26 1325 313 181 181
query27 2676 558 357 357
query28 4364 2037 2016 2016
query29 1008 643 538 538
query30 320 243 201 201
query31 1140 1080 973 973
query32 91 78 77 77
query33 562 406 286 286
query34 1181 1150 661 661
query35 765 792 696 696
query36 1412 1443 1298 1298
query37 153 103 89 89
query38 3171 3168 3145 3145
query39 936 915 901 901
query39_1 874 862 900 862
query40 233 149 126 126
query41 70 62 63 62
query42 118 113 110 110
query43 328 336 287 287
query44
query45 216 204 202 202
query46 1118 1194 753 753
query47 2391 2430 2224 2224
query48 396 426 281 281
query49 634 523 393 393
query50 994 369 265 265
query51 4328 4316 4308 4308
query52 106 105 96 96
query53 248 286 210 210
query54 323 273 258 258
query55 94 94 89 89
query56 333 318 305 305
query57 1459 1446 1318 1318
query58 305 271 268 268
query59 1586 1682 1418 1418
query60 329 327 305 305
query61 158 157 159 157
query62 709 652 587 587
query63 245 205 204 204
query64 2366 790 656 656
query65
query66 1675 475 357 357
query67 29845 29815 28918 28918
query68
query69 453 334 300 300
query70 1077 1013 949 949
query71 324 276 274 274
query72 2989 2688 2429 2429
query73 817 814 410 410
query74 5102 4946 4754 4754
query75 2730 2597 2284 2284
query76 2313 1150 773 773
query77 399 405 336 336
query78 12452 12579 11933 11933
query79 1481 1089 755 755
query80 671 532 467 467
query81 463 279 247 247
query82 1375 160 126 126
query83 360 280 248 248
query84 270 142 115 115
query85 901 544 460 460
query86 402 349 316 316
query87 3474 3366 3207 3207
query88 3635 2719 2722 2719
query89 460 387 343 343
query90 1985 202 183 183
query91 180 173 141 141
query92 84 76 77 76
query93 1469 1414 818 818
query94 553 356 306 306
query95 671 472 347 347
query96 1078 796 369 369
query97 2749 2708 2624 2624
query98 244 244 237 237
query99 1160 1169 1048 1048
Total cold run time: 254746 ms
Total hot run time: 170126 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622400196
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3354877345
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughSetOperation.java:
##########
@@ -61,11 +63,47 @@ public Rule build() {
.when(s -> s.arity() > 0
|| (s instanceof LogicalUnion && !((LogicalUnion) s).getConstantExprsList().isEmpty())))
.thenApply(ctx -> {
- LogicalFilter<LogicalSetOperation> filter = ctx.root;
- LogicalSetOperation setOperation = filter.child();
+ LogicalFilter<LogicalSetOperation> origFilter = ctx.root;
+ LogicalSetOperation setOperation = origFilter.child();
+
+ // Pushing a conjunct that contains a volatile expression (rand/uuid/random_bytes/...)
+ // into each branch changes semantics for every set-op except UNION ALL.
+ // - UNION ALL: each branch row = exactly one output row (1:1), so evaluating
+ // rand() once per branch row still matches the per-output-row semantic.
+ // - UNION DISTINCT / INTERSECT / EXCEPT: the set-op semantics depend on the
+ // full branch row sets before dedup/intersect/except. Sampling rows in each
+ // branch independently changes which rows participate (e.g. INTERSECT becomes
+ // "half of A intersect half of B" instead of "half of (A intersect B)").
+ boolean canPushVolatileExpr = setOperation instanceof LogicalUnion
+ && setOperation.getQualifier() == Qualifier.ALL;
+ Set<Expression> pushableConjuncts;
+ Set<Expression> keptAboveConjuncts;
+ if (canPushVolatileExpr) {
+ pushableConjuncts = origFilter.getConjuncts();
+ keptAboveConjuncts = ImmutableSet.of();
+ } else {
+ pushableConjuncts = new LinkedHashSet<>();
+ Set<Expression> kept = new LinkedHashSet<>();
+ for (Expression c : origFilter.getConjuncts()) {
+ if (c.containsVolatileExpression()) {
+ kept.add(c);
+ } else {
+ pushableConjuncts.add(c);
+ }
+ }
+ keptAboveConjuncts = kept;
+ if (pushableConjuncts.isEmpty()) {
+ return null;
+ }
+ }
+ LogicalFilter<LogicalSetOperation> filter = pushableConjuncts == origFilter.getConjuncts()
Review Comment:
The reference equality check `pushableConjuncts == origFilter.getConjuncts()` determines whether to reuse the original filter or create a new one. While currently correct (because the UNION ALL branch at line 82 assigns `pushableConjuncts = origFilter.getConjuncts()` — the same reference), this is fragile: if `getConjuncts()` were ever changed to return a defensive copy or a view, the `==` check would silently break.
Consider replacing it with an explicit boolean flag, which would be more robust and self-documenting:
```java
boolean allConjunctsPushable = canPushVolatileExpr;
// ...
LogicalFilter<LogicalSetOperation> filter = allConjunctsPushable
? origFilter
: new LogicalFilter<>(ImmutableSet.copyOf(pushableConjuncts), setOperation);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
Review Comment:
The local variable `inputSlots` on line 283 (`Set<Slot> inputSlots = expr.getInputSlots();`) shadows the meaning of `inputSlots` on line 286 which refers to the same variable. This is correct behavior, but consider renaming the inner-scope variable (e.g., `ownInputSlots` or `volatileOwnSlots`) to distinguish the volatile expression's own input slots from the accumulated context slots stored in the map. This would also clarify the intent at line 286 where `inputSlots.isEmpty()` checks the volatile expression's own slots, while `target.getInputSlots()` provides the fallback context from the containing expression.
Suggested rename:
```java
Set<Slot> ownSlots = expr.getInputSlots();
volatileExpressionSlots
.computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
.addAll(ownSlots.isEmpty() ? target.getInputSlots() : ownSlots);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4302850289
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4303743162
# FE Regression Coverage Report
Increment line coverage ` 83.33% (5/6)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_12d07aaad64c4e68b4ef0e2454c448a645b2d21a_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_12d07aaad64c4e68b4ef0e2454c448a645b2d21a_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4598129295
<details>
<summary>TPC-DS: <b>Total hot run time: 170725 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit ece53853ea7620ca5434d974848d02e9ab411db8, data reload: false
query5 4327 663 517 517
query6 351 234 215 215
query7 4271 589 311 311
query8 346 245 227 227
query9 8821 4029 4043 4029
query10 460 351 305 305
query11 5790 2386 2133 2133
query12 189 131 132 131
query13 1343 618 446 446
query14 6062 5478 5125 5125
query14_1 4457 4450 4505 4450
query15 212 210 190 190
query16 993 449 455 449
query17 1164 754 631 631
query18 2471 490 366 366
query19 227 210 172 172
query20 145 142 138 138
query21 220 146 120 120
query22 13681 13668 13436 13436
query23 17393 16599 16203 16203
query23_1 16220 16377 16403 16377
query24 7463 1788 1300 1300
query24_1 1327 1326 1296 1296
query25 611 510 460 460
query26 1315 319 176 176
query27 2703 569 353 353
query28 4429 1995 1985 1985
query29 1055 653 546 546
query30 306 226 190 190
query31 1145 1081 957 957
query32 93 74 75 74
query33 547 355 318 318
query34 1212 1144 669 669
query35 768 799 693 693
query36 1417 1394 1213 1213
query37 158 105 93 93
query38 3199 3132 3062 3062
query39 935 917 904 904
query39_1 884 893 862 862
query40 236 154 129 129
query41 65 63 63 63
query42 121 115 111 111
query43 358 337 287 287
query44
query45 217 207 200 200
query46 1085 1167 743 743
query47 2399 2399 2308 2308
query48 406 422 304 304
query49 627 502 376 376
query50 992 355 253 253
query51 4330 4375 4286 4286
query52 108 113 96 96
query53 258 284 206 206
query54 315 270 249 249
query55 93 93 85 85
query56 300 314 314 314
query57 1459 1438 1323 1323
query58 309 276 268 268
query59 1601 1682 1430 1430
query60 324 337 316 316
query61 159 157 154 154
query62 716 656 581 581
query63 246 206 236 206
query64 2461 782 626 626
query65
query66 1735 482 355 355
query67 29794 29744 29686 29686
query68
query69 473 340 302 302
query70 997 1039 996 996
query71 312 277 268 268
query72 2961 2623 2388 2388
query73 858 767 433 433
query74 5120 4948 4766 4766
query75 2697 2608 2255 2255
query76 2283 1197 778 778
query77 390 420 352 352
query78 12500 12300 11859 11859
query79 1467 1070 761 761
query80 736 519 473 473
query81 478 281 243 243
query82 1380 156 119 119
query83 314 273 244 244
query84 257 139 115 115
query85 907 557 455 455
query86 419 343 328 328
query87 3452 3369 3239 3239
query88 3610 2729 2746 2729
query89 453 397 342 342
query90 1938 187 178 178
query91 179 168 136 136
query92 79 80 72 72
query93 1468 1448 912 912
query94 593 357 308 308
query95 687 488 364 364
query96 1061 859 349 349
query97 2743 2774 2621 2621
query98 241 228 221 221
query99 1153 1186 1056 1056
Total cold run time: 254593 ms
Total hot run time: 170725 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3339616550
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOtherCondition.java:
##########
@@ -75,7 +75,16 @@ public Rule build() {
Set<Expression> rightConjuncts = Sets.newHashSet();
for (Expression otherConjunct : otherJoinConjuncts) {
- if (PUSH_DOWN_LEFT_VALID_TYPE.contains(join.getJoinType())
+ // Keep slot-free volatile predicates in otherJoinConjuncts. Otherwise
+ // `allCoveredBy(..., child.getOutputSet())` would arbitrarily push
+ // `rand() > 0.5` into one side because empty slots are covered by both
+ // children. Side-specific volatile predicates are still pushed down for
+ // performance, matching PostgreSQL's treatment of single-relation INNER
+ // JOIN ON quals as base restrictions.
+ if (otherConjunct.containsVolatileExpression()
Review Comment:
Thanks, addressed in 0e523879874. PushDownJoinOtherCondition now keeps every containsVolatileExpression() ON conjunct in otherJoinConjuncts, including one-sided predicates such as t1.id + rand() > 0.5, so the join-pair evaluation granularity is preserved. For BETWEEN-expanded repeated volatile expressions, AddProjectForVolatileExpression materializes one value later: right-only expressions are projected on the right child, otherwise it falls back to the left child, and volatile expressions whose own slots span both join children are skipped because join has no pair-scope project. I also added FE unit tests and explain-shape regression cases for the one-sided volatile ON predicate and left/right/slot-free BETWEEN cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4601518883
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3361059015
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> volatileInputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(volatileInputSlots.isEmpty() ? target.getInputSlots() : volatileInputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
+ }
+ Set<Slot> inputSlots = volatileExpressionSlots.get(entry.getKey());
+ Set<Slot> volatileInputSlots = entry.getKey().getInputSlots();
+ if (!volatileInputSlots.isEmpty()
+ && !leftOutputSet.containsAll(inputSlots)
+ && !rightOutputSet.containsAll(inputSlots)) {
+ continue;
+ }
+ ExprId exprId = StatementScopeIdGenerator.newExprId();
+ String functionName = entry.getKey() instanceof Function
+ ? ((Function) entry.getKey()).getName() : "volatile";
+ Alias alias = new Alias(exprId, entry.getKey(), "$_" + functionName + "_" + exprId.asInt() + "_$");
+ replaceMap.put(alias.child(0), alias.toSlot());
+ // Join can not add a project at join-pair scope, but repeated volatile expressions
+ // still need one materialized value. Slot-free volatile functions use the containing
+ // conjunct's slots to choose a side, so t2.k + rand() can project rand() on the right.
+ // Volatile functions with input slots use their own slots to avoid projecting
+ // volatile_udf(t2.k) on the left only because its containing conjunct also uses t1.
+ // Volatile functions whose own slots span both join children cannot be projected into
+ // either child, so they are not rewritten here.
+ // Put right-only expressions on the right child; otherwise keep the previous
+ // left-child behavior as the conservative default.
+ if (!inputSlots.isEmpty() && rightOutputSet.containsAll(inputSlots)) {
+ rightAliases.add(alias);
+ } else {
Review Comment:
This is the same trade-off discussed earlier.
I agree that child-side materialization is not perfectly equivalent to join-pair evaluation. But simply skipping this rewrite is not safe either, because after BETWEEN expansion a repeated volatile expression is evaluated multiple times on the join.
For example, `rand() between 0.6 and 0.1` should never be true if the same rand value is used. After expansion, leaving it as `rand() >= 0.6 AND rand() <= 0.1` evaluates `rand()` twice, so it can incorrectly become true.
Since the current plan model cannot materialize a volatile expression at join-pair scope, we have two imperfect choices:
1. leave duplicated volatile expressions on the join and allow internally inconsistent predicate evaluation;
2. materialize once in a child, preserving single-value semantics but changing evaluation granularity.
The current implementation chooses the second as the less-bad behavior. A fully correct fix would require either preserving volatile BETWEEN before expansion or adding join-pair-scope materialization support. So I do not think simply skipping repeated volatile ON expressions is a correctness improvement; it just switches back to the duplicated-evaluation bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622369610
Reply to review: the `AddProjectForVolatileExpression` rename from `inputSlots` to `volatileInputSlots` is only to remove the naming ambiguity in the nested loop that collects repeated volatile expressions. No behavior changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4647118502
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4304365590
# FE UT Coverage Report
Increment line coverage ` 66.67% (6/9)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_310308d6653689d366b0b411c7822907667eebf4/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_310308d6653689d366b0b411c7822907667eebf4/fe_report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4304655585
# FE Regression Coverage Report
Increment line coverage ` 88.89% (8/9)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_310308d6653689d366b0b411c7822907667eebf4_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_310308d6653689d366b0b411c7822907667eebf4_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3354706421
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
实际上，之前的代码，在rand() 出现多次时，无脑落入左边，其实也是没问题
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4620781772
## PR Description Review
The PR description mentions several rewrite rules that are **not present in this diff**:
- **PushDownFilterThroughRepeat** — not modified
- **PushDownFilterThroughSort** — not modified
- **PushDownTopNThroughUnion** / **PushDownTopNDistinctThroughUnion** — not modified
Please update the description to accurately reflect only the rules changed in this PR to avoid confusion about whether changes are missing or deferred to a later batch. The actual changes cover:
- PushDownFilterThroughPartitionTopN ✅
- PushDownFilterThroughWindow ✅
- PushDownFilterThroughSetOperation ✅
- PushDownJoinOtherCondition ✅
- ProjectOtherJoinConditionForNestedLoopJoin ✅
- AddProjectForVolatileExpression ✅
- InferPredicates ✅
- JoinUtils.isHashJoinCondition ✅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622842466
<details>
<summary>TPC-DS: <b>Total hot run time: 169182 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 4639cadc23aed6d31474a1b45c463a6b10093534, data reload: false
query5 4305 631 494 494
query6 436 199 180 180
query7 4809 592 318 318
query8 365 215 204 204
query9 8803 4074 4079 4074
query10 446 307 271 271
query11 5937 2329 2182 2182
query12 159 104 101 101
query13 1278 600 430 430
query14 6359 5469 5067 5067
query14_1 4433 4409 4721 4409
query15 204 197 178 178
query16 987 462 419 419
query17 1099 685 572 572
query18 2444 467 340 340
query19 199 180 139 139
query20 111 108 105 105
query21 219 140 119 119
query22 13752 13559 13417 13417
query23 17277 16557 16203 16203
query23_1 16274 16206 16390 16206
query24 7441 1740 1298 1298
query24_1 1344 1314 1303 1303
query25 566 469 414 414
query26 1314 323 174 174
query27 2665 554 338 338
query28 4488 2079 2053 2053
query29 1090 631 519 519
query30 311 241 201 201
query31 1154 1077 967 967
query32 109 64 60 60
query33 550 328 262 262
query34 1180 1093 657 657
query35 775 803 692 692
query36 1380 1370 1233 1233
query37 155 111 93 93
query38 3190 3158 3064 3064
query39 937 931 908 908
query39_1 906 874 900 874
query40 222 128 105 105
query41 72 67 67 67
query42 99 99 99 99
query43 333 337 287 287
query44
query45 197 187 186 186
query46 1084 1199 735 735
query47 2389 2369 2235 2235
query48 401 399 312 312
query49 677 491 360 360
query50 1038 351 257 257
query51 4342 4315 4242 4242
query52 94 91 79 79
query53 241 285 204 204
query54 294 237 211 211
query55 80 78 72 72
query56 247 255 233 233
query57 1420 1413 1318 1318
query58 249 226 214 214
query59 1623 1667 1438 1438
query60 317 258 248 248
query61 189 204 152 152
query62 696 662 582 582
query63 224 189 180 180
query64 2553 784 609 609
query65
query66 1783 465 349 349
query67 29805 29656 29522 29522
query68
query69 439 301 270 270
query70 972 972 946 946
query71 295 228 220 220
query72 2998 2683 2442 2442
query73 863 743 431 431
query74 5188 4966 4805 4805
query75 2660 2591 2230 2230
query76 2309 1148 803 803
query77 362 364 286 286
query78 12483 12530 11855 11855
query79 1409 1126 765 765
query80 1269 477 388 388
query81 522 279 236 236
query82 622 161 116 116
query83 318 276 253 253
query84 257 144 112 112
query85 896 535 419 419
query86 434 295 285 285
query87 3418 3435 3197 3197
query88 3714 2764 2760 2760
query89 445 376 328 328
query90 1882 185 188 185
query91 175 170 138 138
query92 64 61 61 61
query93 1576 1423 862 862
query94 728 359 307 307
query95 663 383 455 383
query96 1040 790 338 338
query97 2713 2690 2532 2532
query98 214 215 201 201
query99 1148 1180 1028 1028
Total cold run time: 252469 ms
Total hot run time: 169182 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4620376074
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4591278324
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4592647067
# FE Regression Coverage Report
Increment line coverage ` 23.58% (50/212)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_141e4bfc520388c1f72728f0004eab77e5fd35f0_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_141e4bfc520388c1f72728f0004eab77e5fd35f0_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4603217528
# FE Regression Coverage Report
Increment line coverage ` 72.30% (107/148)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_10ca6e7edcbcd5d69aebf8b4bdd4b7c7cd667975_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_10ca6e7edcbcd5d69aebf8b4bdd4b7c7cd667975_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3128992800
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNThroughUnion.java:
##########
@@ -61,6 +61,8 @@ public class PushDownTopNThroughUnion implements RewriteRuleFactory {
public List<Rule> buildRules() {
return ImmutableList.of(
logicalTopN(logicalUnion().when(union -> union.getQualifier() == Qualifier.ALL))
+ .when(topN -> topN.getOrderKeys().stream()
+ .noneMatch(key -> key.getExpr().containsUniqueFunction()))
Review Comment:
There's still a parallel set-operation rewrite with the same issue: `PushDownFilterThroughSetOperation` copies every conjunct into every child without checking `containsUniqueFunction()`. For `SELECT * FROM (SELECT 1 UNION SELECT 1) u WHERE rand() > 0.5`, the original plan evaluates `rand()` once after `UNION DISTINCT` (keep probability 0.5), but the rewritten plan evaluates it once per branch and the row survives if either branch passes (probability 0.75). `INTERSECT` and `EXCEPT` have the same problem, so this batch still leaves a wrong-result path open.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughRepeat.java:
##########
@@ -72,7 +72,8 @@ public Rule build() {
Set<Expression> notPushedPredicates = Sets.newHashSet();
for (Expression conjunct : filter.getConjuncts()) {
Set<Slot> conjunctSlots = conjunct.getInputSlots();
- if (commonGroupingSetExpressions.containsAll(conjunctSlots)) {
+ if (commonGroupingSetExpressions.containsAll(conjunctSlots)
+ && !conjunct.containsUniqueFunction()) {
Review Comment:
Same non-idempotent gap still exists in `PushDownJoinOtherCondition`. That rule only checks `predicate.getInputSlots()`, so `ON rand() > 0.5` (empty slot set) and `ON t1.id + rand() > 5` are still pushed to one child for inner/cross joins. That changes evaluation from once per joined row to once per left/right input row: with one left row joining two right rows, the original plan can keep one pair and drop the other, while the rewritten plan can only keep both or none. Since this PR is fixing exactly this class of unique-function pushdown bug, I think we need the same `containsUniqueFunction()` guard there as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4303614180
Thanks for the reviews. Consolidated response:
1. **`PushDownJoinOtherCondition` – false positive.** The current HEAD of this PR already adds the `containsUniqueFunction()` guard at `PushDownJoinOtherCondition.java:85-87`. Any conjunct containing a unique function is routed into `remainingOther` and is never pushed to a single child. The empirical shape I verified before merging matches this: `ON rand() > 0.5` and `ON t1.id + rand() > 5` stay in the join's other-conjuncts list. The review comment appears to have analyzed the pre-change version of the file.
2. **`PushDownFilterThroughSetOperation` – valid and already fixed in a follow-up.** This is the batch-2 rule in the audit and is addressed in #62754, which splits conjuncts so that `UniqueFunction` predicates above `UNION DISTINCT` / `INTERSECT` / `EXCEPT` remain above the set op while safe conjuncts continue to push down. Regression coverage for EXCEPT / INTERSECT / UNION DISTINCT with `rand()` / `uuid()` is added there.
3. **Normalized-slot NLJ path (`ProjectOtherJoinConditionForNestedLoopJoin` → later `PUSH_DOWN_FILTERS`).** The root cause is Doris' `BETWEEN` rewrite expanding `expr BETWEEN a AND b` into `expr >= a AND expr <= b`, duplicating any `UniqueFunction` inside `expr`. Once duplicated, the later `AddProjectForUniqueFunction.JoinRewrite` materializes one alias into the left child project to keep the two bound checks consistent with a single value. Both options (keep duplicates inline per-pair, or alias into a child) lose some part of the original `BETWEEN` semantics that was already destroyed at the split step; the current choice is the less-bad option in practice. Full discussion is in the #62754 reply. This is a pre-existing trade-off in `AddProjectForUniqueFunction`, not something introduced by this PR's changes to `PushDownJoinOtherCondition` / `PushDownFilterThroughWindow` / `PushDownFilterThroughPartitionTopN`, so it is intentionally out of scope here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3129526808
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOtherCondition.java:
##########
@@ -75,7 +75,16 @@ public Rule build() {
Set<Expression> rightConjuncts = Sets.newHashSet();
for (Expression otherConjunct : otherJoinConjuncts) {
- if (PUSH_DOWN_LEFT_VALID_TYPE.contains(join.getJoinType())
+ // A conjunct that contains a unique (non-idempotent) function such as
+ // rand()/uuid() must NOT be pushed into a single child. Evaluating it
+ // per-input-row instead of per-join-pair changes semantics (every
+ // surviving row has the filter re-drawn once instead of once per pair).
+ // In addition, a predicate like `rand() > 0.5` has empty input slots,
+ // so `allCoveredBy(..., child.getOutputSet())` would otherwise wrongly
+ // return true because `Set.containsAll(emptySet) == true`.
+ if (otherConjunct.containsUniqueFunction()) {
Review Comment:
This fixes the direct AST case, but I think a normalized-slot path is still open. `ProjectOtherJoinConditionForNestedLoopJoin` can later rewrite a one-sided predicate like `t2.id * random(1, 100) between 100 and 200` into a child-local boolean slot; the final `PUSH_DOWN_FILTERS` pass then sees only that slot and pushes it into the child because `containsUniqueFunction()` is now false. The updated `regression-test/data/nereids_rules_p0/unique_function/add_project_for_unique_function.out` still shows exactly that for `join_1` (and likewise pushes `random(1, 100) between 1 and 10` to the left child), which changes evaluation from once per joined pair to once per input row. This is distinct from the already-raised direct `containsUniqueFunction()` thread because the unique function has already been hidden behind a slot by the time the later pushdown runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3356110310
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
I think this is the less-bad behavior under Doris's current join/project split. A cannot represent a per-pair project for a volatile expression, so if the same volatile node appears multiple times after rewrites like expansion, leaving it in the join means it will be re-evaluated independently for each occurrence and can diverge within the same predicate. By materializing it into one child project, we at least force repeated references to share a single value, which preserves the one-value-per-expression property that the original predicate needs. This is not fully equivalent to the join-pair semantics, but it is strictly better than keeping the duplicated volatile expression on the join and letting it be re-sampled separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4623009564
# FE UT Coverage Report
Increment line coverage ` 83.33% (100/120)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_4639cadc23aed6d31474a1b45c463a6b10093534/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_4639cadc23aed6d31474a1b45c463a6b10093534/fe_report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3356155213
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughSetOperation.java:
##########
@@ -61,11 +63,47 @@ public Rule build() {
.when(s -> s.arity() > 0
|| (s instanceof LogicalUnion && !((LogicalUnion) s).getConstantExprsList().isEmpty())))
.thenApply(ctx -> {
- LogicalFilter<LogicalSetOperation> filter = ctx.root;
- LogicalSetOperation setOperation = filter.child();
+ LogicalFilter<LogicalSetOperation> origFilter = ctx.root;
+ LogicalSetOperation setOperation = origFilter.child();
+
+ // Pushing a conjunct that contains a volatile expression (rand/uuid/random_bytes/...)
+ // into each branch changes semantics for every set-op except UNION ALL.
+ // - UNION ALL: each branch row = exactly one output row (1:1), so evaluating
+ // rand() once per branch row still matches the per-output-row semantic.
+ // - UNION DISTINCT / INTERSECT / EXCEPT: the set-op semantics depend on the
+ // full branch row sets before dedup/intersect/except. Sampling rows in each
+ // branch independently changes which rows participate (e.g. INTERSECT becomes
+ // "half of A intersect half of B" instead of "half of (A intersect B)").
+ boolean canPushVolatileExpr = setOperation instanceof LogicalUnion
+ && setOperation.getQualifier() == Qualifier.ALL;
+ Set<Expression> pushableConjuncts;
+ Set<Expression> keptAboveConjuncts;
+ if (canPushVolatileExpr) {
+ pushableConjuncts = origFilter.getConjuncts();
+ keptAboveConjuncts = ImmutableSet.of();
+ } else {
+ pushableConjuncts = new LinkedHashSet<>();
+ Set<Expression> kept = new LinkedHashSet<>();
+ for (Expression c : origFilter.getConjuncts()) {
+ if (c.containsVolatileExpression()) {
+ kept.add(c);
+ } else {
+ pushableConjuncts.add(c);
+ }
+ }
+ keptAboveConjuncts = kept;
+ if (pushableConjuncts.isEmpty()) {
+ return null;
+ }
+ }
+ LogicalFilter<LogicalSetOperation> filter = pushableConjuncts == origFilter.getConjuncts()
Review Comment:
This is a robustness cleanup only. The previous reference-equality check worked because the UNION ALL branch reused the original set instance, but that dependency was implicit and brittle. Using an explicit boolean makes the intent clear and avoids depending on getConjuncts() preserving reference identity.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
Review Comment:
This rename only removes the local naming ambiguity in the nested loop that collects repeated volatile expressions. No behavior changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4623782190
# FE Regression Coverage Report
Increment line coverage ` 92.50% (111/120)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_4639cadc23aed6d31474a1b45c463a6b10093534_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_4639cadc23aed6d31474a1b45c463a6b10093534_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4603225055
<details>
<summary>TPC-DS: <b>Total hot run time: 168563 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 0e523879874e8209bb77e968800affbbade23fc5, data reload: false
query5 4320 630 469 469
query6 450 203 182 182
query7 4801 564 321 321
query8 371 213 198 198
query9 8736 4077 4025 4025
query10 451 312 254 254
query11 5911 2354 2185 2185
query12 157 100 105 100
query13 1259 628 440 440
query14 6997 5432 5126 5126
query14_1 4368 4383 4370 4370
query15 202 192 174 174
query16 1005 497 422 422
query17 1093 692 578 578
query18 2698 476 331 331
query19 205 178 144 144
query20 110 104 105 104
query21 213 138 118 118
query22 13608 13580 13381 13381
query23 17345 16498 16123 16123
query23_1 16338 16286 16359 16286
query24 7527 1806 1281 1281
query24_1 1331 1330 1269 1269
query25 546 450 408 408
query26 1303 303 173 173
query27 2651 563 342 342
query28 4455 2045 2060 2045
query29 1113 639 518 518
query30 315 237 200 200
query31 1138 1088 976 976
query32 109 63 60 60
query33 538 337 271 271
query34 1175 1155 643 643
query35 765 776 685 685
query36 1371 1389 1264 1264
query37 151 114 93 93
query38 3219 3180 3102 3102
query39 929 922 891 891
query39_1 887 880 879 879
query40 240 129 110 110
query41 75 70 68 68
query42 99 99 96 96
query43 321 337 279 279
query44
query45 199 193 179 179
query46 1095 1231 749 749
query47 2402 2357 2277 2277
query48 408 437 311 311
query49 643 482 371 371
query50 992 383 266 266
query51 4391 4333 4190 4190
query52 94 92 87 87
query53 256 280 192 192
query54 286 238 214 214
query55 81 76 77 76
query56 262 234 251 234
query57 1415 1403 1307 1307
query58 248 228 221 221
query59 1627 1681 1485 1485
query60 303 268 247 247
query61 190 191 188 188
query62 696 655 599 599
query63 240 230 202 202
query64 2512 791 637 637
query65
query66 1734 457 342 342
query67 29993 29733 28891 28891
query68
query69 419 305 269 269
query70 957 959 909 909
query71 310 217 216 216
query72 3021 2697 1985 1985
query73 837 804 425 425
query74 5091 4939 4738 4738
query75 2666 2563 2227 2227
query76 2327 1160 776 776
query77 347 380 316 316
query78 12432 12501 11921 11921
query79 1486 1063 783 783
query80 1313 475 392 392
query81 517 283 245 245
query82 617 159 122 122
query83 360 280 250 250
query84 269 141 110 110
query85 928 525 429 429
query86 424 309 291 291
query87 3443 3355 3174 3174
query88 3678 2764 2722 2722
query89 438 379 339 339
query90 1896 188 184 184
query91 178 173 141 141
query92 67 64 61 61
query93 1676 1396 934 934
query94 712 353 307 307
query95 694 470 341 341
query96 1119 809 367 367
query97 2685 2714 2537 2537
query98 210 203 200 200
query99 1157 1202 1045 1045
Total cold run time: 253698 ms
Total hot run time: 168563 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4608980485
# FE UT Coverage Report
Increment line coverage ` 83.05% (98/118)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_495775c875ff389976dcf7fec551953b47d5665a/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_495775c875ff389976dcf7fec551953b47d5665a/fe_report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3130357914
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -120,11 +120,14 @@ public Rule build() {
Set<Expression> rightPredicates = Sets.newLinkedHashSet();
Set<Expression> remainingPredicates = Sets.newLinkedHashSet();
for (Expression p : filterPredicates) {
- if (p.containsUniqueFunction()) {
+ Set<Slot> slots = p.collect(SlotReference.class::isInstance);
+ if (p.containsUniqueFunction() && slots.isEmpty()) {
Review Comment:
This guard is still too narrow. A one-sided predicate like above a is evaluated once per joined row in the original plan, but after pushing it into the right child it is evaluated once per row and then reused for every matching left row. With one right row joining two left rows, the original plan can keep one pair and drop the other, while the rewritten plan can only keep both or none. That makes this a wrong-result path distinct from the already-discussed thread, and the updated regression outputs now lock in that unsafe plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4302262935
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4598082909
<details>
<summary>TPC-H: <b>Total hot run time: 28968 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit ece53853ea7620ca5434d974848d02e9ab411db8, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17637 4039 4020 4020
q2 q3 10740 1381 803 803
q4 4696 472 352 352
q5 7519 865 599 599
q6 180 171 138 138
q7 779 862 633 633
q8 9418 1489 1532 1489
q9 5746 4511 4512 4511
q10 6776 1807 1568 1568
q11 443 276 249 249
q12 633 427 288 288
q13 18086 3408 2774 2774
q14 267 260 241 241
q15 q16 808 776 708 708
q17 952 935 937 935
q18 6931 5807 5555 5555
q19 1321 1257 1072 1072
q20 504 400 263 263
q21 5927 2600 2466 2466
q22 421 354 304 304
Total cold run time: 99784 ms
Total hot run time: 28968 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 4323 4263 4262 4262
q2 q3 4535 4968 4316 4316
q4 2086 2241 1393 1393
q5 4431 4338 4331 4331
q6 231 175 129 129
q7 1748 1646 1593 1593
q8 2878 2258 2165 2165
q9 8184 8217 8090 8090
q10 4802 4803 4276 4276
q11 568 410 387 387
q12 751 773 544 544
q13 3343 3691 3042 3042
q14 303 311 288 288
q15 q16 706 722 646 646
q17 1345 1327 1323 1323
q18 8188 7456 7321 7321
q19 1218 1149 1150 1149
q20 2232 2240 1999 1999
q21 5305 4571 4485 4485
q22 503 468 437 437
Total cold run time: 57680 ms
Total hot run time: 52176 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4609842150
# FE Regression Coverage Report
Increment line coverage ` 40.98% (109/266)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62742_495775c875ff389976dcf7fec551953b47d5665a_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62742_495775c875ff389976dcf7fec551953b47d5665a_merge_fe/report/index.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4600039086
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4622369725
Reply to review: the `PushDownFilterThroughSetOperation` change uses an explicit `allConjunctsPushable` flag instead of relying on reference equality. The old code happened to work because the UNION ALL branch reused the original set instance, but that dependency was implicit and brittle. Making the condition explicit is safer and self-documenting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4603699548
<details>
<summary>TPC-H: <b>Total hot run time: 29142 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 10ca6e7edcbcd5d69aebf8b4bdd4b7c7cd667975, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17601 4027 4023 4023
q2 q3 10777 1385 798 798
q4 4686 479 346 346
q5 7724 899 591 591
q6 179 173 137 137
q7 780 862 660 660
q8 9341 1696 1562 1562
q9 5949 4491 4505 4491
q10 6728 1805 1547 1547
q11 439 271 251 251
q12 632 433 288 288
q13 18105 3373 2765 2765
q14 260 257 249 249
q15 q16 816 783 707 707
q17 949 968 973 968
q18 6885 5720 5476 5476
q19 1328 1226 1039 1039
q20 518 400 259 259
q21 6179 2867 2675 2675
q22 470 389 310 310
Total cold run time: 100346 ms
Total hot run time: 29142 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 5058 4871 4791 4791
q2 q3 4864 5255 4671 4671
q4 2142 2196 1391 1391
q5 4777 4809 4717 4717
q6 229 175 126 126
q7 1842 1791 1594 1594
q8 2402 2109 2102 2102
q9 7848 7652 7457 7457
q10 4738 4641 4207 4207
q11 532 390 355 355
q12 740 733 536 536
q13 3007 3321 2831 2831
q14 279 289 256 256
q15 q16 681 697 613 613
q17 1286 1254 1250 1250
q18 7243 6691 6842 6691
q19 1123 1094 1121 1094
q20 2214 2226 1937 1937
q21 5320 4641 4478 4478
q22 523 453 394 394
Total cold run time: 56848 ms
Total hot run time: 51491 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on code in PR #62742:
URL: https://github.com/apache/doris/pull/62742#discussion_r3354691735
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AddProjectForVolatileExpression.java:
##########
@@ -269,6 +271,85 @@ public <T extends Expression> Optional<Pair<List<T>, LogicalProject<Plan>>> rewr
return Optional.of(Pair.of(newTargetsBuilder.build(), new LogicalProject<>(projects, plan.child(0))));
}
+ private Optional<JoinRewriteResult> rewriteJoinExpressions(LogicalJoin<Plan, Plan> join,
+ Collection<Expression> targets) {
+ Map<Expression, Integer> volatileExpressionCounter = Maps.newLinkedHashMap();
+ Map<Expression, Set<Slot>> volatileExpressionSlots = Maps.newLinkedHashMap();
+ for (Expression target : targets) {
+ target.foreach(e -> {
+ Expression expr = (Expression) e;
+ if (expr.isVolatile()) {
+ volatileExpressionCounter.merge(expr, 1, Integer::sum);
+ Set<Slot> inputSlots = expr.getInputSlots();
+ volatileExpressionSlots
+ .computeIfAbsent(expr, ignored -> Sets.newLinkedHashSet())
+ .addAll(inputSlots.isEmpty() ? target.getInputSlots() : inputSlots);
+ }
+ });
+ }
+
+ ImmutableList.Builder<NamedExpression> leftAliases = ImmutableList.builder();
+ ImmutableList.Builder<NamedExpression> rightAliases = ImmutableList.builder();
+ Map<Expression, Slot> replaceMap = Maps.newHashMap();
+ Set<Slot> leftOutputSet = join.left().getOutputSet();
+ Set<Slot> rightOutputSet = join.right().getOutputSet();
+ for (Entry<Expression, Integer> entry : volatileExpressionCounter.entrySet()) {
+ if (entry.getValue() <= 1) {
+ continue;
Review Comment:
这个是没办法的。如果rand() 不下推， 留在join 上，此时这个rand() 被错误计算两次了，这种情况更加不可接受。相比rand() 被错误计算两次， 下推到任何一边的结果都是更加可接受的
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](nereids) Guard UniqueFunction in multiple filter/topn pushdown rules [doris]
Posted by "morrySnow (via GitHub)" <gi...@apache.org>.
morrySnow commented on PR #62742:
URL: https://github.com/apache/doris/pull/62742#issuecomment-4628826315
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org