[BugFix] MaxBy/MinBy not filter nulls #51354

satanson · 2024-09-24T13:08:08Z

Why I'm doing:

max_by/min_by should not filter nulls, for an example:

prepare data

create table test_table (
   `id` int NOT NULL,
   `name` varchar(10) DEFAULT NULL,
   `grade` int NOT NULL
) ENGINE=OLAP
PRIMARY KEY(`id`)
DISTRIBUTED BY HASH(`id`);

insert into test_table values (1,'john',90);
insert into test_table values (2,'jim',95);
insert into test_table values (3,NULL,100);

query Expected outcome 'null' actual outcome 'jim'

select max_by(name, grade) from test_table;

What I'm doing:

max_by and min_by not filter nulls;
fix ancient bug: crash caused by window query like max_by(T, S) over window_def when S's type is not same to T's

Fixes #issue

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

Signed-off-by: satanson <[email protected]>

sonarqubecloud · 2024-09-26T13:09:53Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-09-26T14:35:01Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-09-26T14:38:59Z

[FE Incremental Coverage Report]

✅ pass : 21 / 21 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/catalog/combinator/AggStateDesc.java	1	1	100.00%	[]
🔵	com/starrocks/analysis/FunctionName.java	1	1	100.00%	[]
🔵	com/starrocks/catalog/Function.java	19	19	100.00%	[]

github-actions · 2024-09-26T14:45:52Z

[BE Incremental Coverage Report]

❌ fail : 231 / 335 (68.96%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/exprs/agg/maxmin_by.h	218	322	67.70%	[98, 99, 100, 101, 112, 113, 116, 117, 118, 121, 122, 123, 124, 125, 129, 130, 132, 133, 134, 135, 137, 138, 139, 142, 143, 144, 145, 185, 186, 187, 188, 199, 200, 203, 204, 205, 208, 209, 210, 211, 212, 216, 217, 219, 220, 221, 222, 224, 225, 226, 229, 230, 231, 232, 239, 256, 312, 313, 314, 360, 361, 362, 421, 422, 423, 425, 426, 428, 451, 453, 455, 456, 457, 458, 460, 461, 463, 482, 483, 484, 525, 526, 527, 528, 548, 549, 551, 553, 554, 555, 556, 558, 559, 561, 583, 693, 694, 695, 739, 740, 741, 742, 786, 789]
🔵	be/src/exec/analytor.cpp	7	7	100.00%	[]
🔵	be/src/exprs/agg/factory/aggregate_factory.cpp	2	2	100.00%	[]
🔵	be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp	4	4	100.00%	[]

ZiheLiu · 2024-09-27T03:14:43Z

be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp

-                        "max_by", true, AggregateFactory::MakeMaxByAggregateFunction<arg_type>());
+                resolver->add_aggregate_mapping_notnull<arg_type, ret_type>(
+                        "max_by", true, AggregateFactory::MakeMaxByAggregateFunction<arg_type, false>());
+                resolver->add_aggregate_mapping_notnull<arg_type, ret_type>(


Why using add_aggregate_mapping_notnull? What will happen if y is a nullable column for max_by(x, y) ?

Oh, it seems that nullable columns are handled by MaxBy itself now.

yes, null is acceptable output values, so max_by/min_by agg cannot be wrapped by NullableAggFunction

be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp

be/src/exec/analytor.cpp

github-actions · 2024-09-27T09:06:17Z

@Mergifyio backport branch-3.3

mergify · 2024-09-27T09:06:41Z

backport branch-3.3

✅ Backports have been created

#51504 [BugFix] MaxBy/MinBy not filter nulls (backport #51354) has been created for branch branch-3.3 but encountered conflicts

mergify · 2024-09-27T09:06:43Z

backport branch-3.2

✅ Backports have been created

#51505 [BugFix] MaxBy/MinBy not filter nulls (backport #51354) has been created for branch branch-3.2 but encountered conflicts

mergify · 2024-09-27T09:06:45Z

backport branch-3.1

✅ Backports have been created

#51506 [BugFix] MaxBy/MinBy not filter nulls (backport #51354) has been created for branch branch-3.1 but encountered conflicts

mergify · 2024-09-27T09:06:46Z

backport branch-3.0

✅ Backports have been created

#51507 [BugFix] MaxBy/MinBy not filter nulls (backport #51354) has been created for branch branch-3.0 but encountered conflicts

mergify · 2024-09-27T09:06:47Z

backport branch-2.5

✅ Backports have been created

#51508 [BugFix] MaxBy/MinBy not filter nulls (backport #51354) has been created for branch branch-2.5 but encountered conflicts

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) # Conflicts: # be/src/exprs/agg/factory/aggregate_factory.cpp # fe/fe-core/src/main/java/com/starrocks/catalog/combinator/AggStateDesc.java # test/sql/test_agg_state/R/test_agg_state_table_with_all_functions.sql

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) # Conflicts: # be/src/exprs/agg/factory/aggregate_factory.cpp # fe/fe-core/src/main/java/com/starrocks/catalog/Function.java # fe/fe-core/src/main/java/com/starrocks/catalog/combinator/AggStateDesc.java # test/sql/test_agg_state/R/test_agg_state_table_with_all_functions.sql

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) # Conflicts: # be/src/exprs/agg/factory/aggregate_factory.cpp # be/src/exprs/agg/factory/aggregate_factory.hpp # be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp # be/src/exprs/agg/maxmin_by.h # be/test/exprs/agg/aggregate_test.cpp # fe/fe-core/src/main/java/com/starrocks/catalog/Function.java # fe/fe-core/src/main/java/com/starrocks/catalog/FunctionSet.java # fe/fe-core/src/main/java/com/starrocks/catalog/combinator/AggStateDesc.java # test/sql/test_agg_state/R/test_agg_state_table_with_all_functions.sql

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) # Conflicts: # be/src/exec/analytor.cpp # be/src/exprs/agg/factory/aggregate_factory.cpp # be/src/exprs/agg/factory/aggregate_factory.hpp # be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp # be/src/exprs/agg/maxmin_by.h # be/test/exprs/agg/aggregate_test.cpp # fe/fe-core/src/main/java/com/starrocks/analysis/FunctionName.java # fe/fe-core/src/main/java/com/starrocks/catalog/Function.java # fe/fe-core/src/main/java/com/starrocks/catalog/FunctionSet.java # fe/fe-core/src/main/java/com/starrocks/catalog/combinator/AggStateDesc.java # test/sql/test_agg_state/R/test_agg_state_table_with_all_functions.sql

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]> Co-authored-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]> Co-authored-by: satanson <[email protected]>

Signed-off-by: satanson <[email protected]> Signed-off-by: zhiminr.ren <[email protected]>

satanson requested review from a team as code owners September 24, 2024 13:08

github-actions bot added 3.3 3.2 3.1 3.0 2.5 labels Sep 24, 2024

mergify bot assigned satanson Sep 24, 2024

satanson force-pushed the max_min_by_not_filter_nulls branch from d83b06c to 5947e16 Compare September 25, 2024 11:47

satanson changed the title ~~[Enhancement] MaxBy/MinBy not filter nulls~~ [UT] MaxBy/MinBy not filter nulls Sep 25, 2024

satanson force-pushed the max_min_by_not_filter_nulls branch from 5947e16 to b20d5d6 Compare September 26, 2024 02:43

[Enhancement] MaxBy/MinBy not filter nulls

2a28107

Signed-off-by: satanson <[email protected]>

satanson force-pushed the max_min_by_not_filter_nulls branch from b20d5d6 to 2a28107 Compare September 26, 2024 13:01

satanson changed the title ~~[UT] MaxBy/MinBy not filter nulls~~ [Debug] MaxBy/MinBy not filter nulls Sep 27, 2024

github-actions bot added the title needs [type] label Sep 27, 2024

stdpain changed the title ~~[Debug] MaxBy/MinBy not filter nulls~~ [BugFix] MaxBy/MinBy not filter nulls Sep 27, 2024

github-actions bot removed the title needs [type] label Sep 27, 2024

ZiheLiu reviewed Sep 27, 2024

View reviewed changes

LiShuMing approved these changes Sep 27, 2024

View reviewed changes

ZiheLiu reviewed Sep 27, 2024

View reviewed changes

be/src/exprs/agg/factory/aggregate_resolver_minmaxany.cpp Show resolved Hide resolved

stdpain reviewed Sep 27, 2024

View reviewed changes

be/src/exec/analytor.cpp Show resolved Hide resolved

stdpain approved these changes Sep 27, 2024

View reviewed changes

kangkaisen merged commit 9398edd into StarRocks:main Sep 27, 2024
86 of 90 checks passed

github-actions bot removed the 3.3 label Sep 27, 2024

github-actions bot removed the 2.5 label Sep 27, 2024

mergify bot mentioned this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51504

Merged

42 tasks

mergify bot mentioned this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51505

Closed

42 tasks

mergify bot mentioned this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51506

Merged

42 tasks

mergify bot mentioned this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51507

Closed

42 tasks

mergify bot mentioned this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51508

Closed

42 tasks

satanson added a commit that referenced this pull request Sep 27, 2024

[BugFix] MaxBy/MinBy not filter nulls (#51354)

5ad1502

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

satanson added a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (#51354)

7bdae5e

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

satanson added a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354)

7a3276f

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

satanson mentioned this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) #51527

Merged

42 tasks

satanson added a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354)

b81c5d8

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

wanpengfei-git pushed a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) (#51504)

a7100a2

Signed-off-by: satanson <[email protected]> Co-authored-by: satanson <[email protected]>

github-actions bot added the 3.3-merged label Sep 29, 2024

satanson added a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354)

1154fe4

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

Seaven pushed a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) (#51527)

1d3273e

Signed-off-by: satanson <[email protected]>

github-actions bot added the 3.2-merged label Sep 29, 2024

satanson added a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354)

e575f51

Signed-off-by: satanson <[email protected]> (cherry picked from commit 9398edd) Signed-off-by: satanson <[email protected]>

wanpengfei-git pushed a commit that referenced this pull request Sep 29, 2024

[BugFix] MaxBy/MinBy not filter nulls (backport #51354) (#51506)

3786862

Signed-off-by: satanson <[email protected]> Co-authored-by: satanson <[email protected]>

github-actions bot added the 3.1-merged label Sep 29, 2024

renzhimin7 pushed a commit to renzhimin7/starrocks that referenced this pull request Nov 7, 2024

[BugFix] MaxBy/MinBy not filter nulls (StarRocks#51354)

966b04c

Signed-off-by: satanson <[email protected]> Signed-off-by: zhiminr.ren <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] MaxBy/MinBy not filter nulls #51354

[BugFix] MaxBy/MinBy not filter nulls #51354

satanson commented Sep 24, 2024 •

edited

Loading

sonarqubecloud bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

ZiheLiu Sep 27, 2024

ZiheLiu Sep 27, 2024

satanson Sep 27, 2024

github-actions bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

[BugFix] MaxBy/MinBy not filter nulls #51354

[BugFix] MaxBy/MinBy not filter nulls #51354

Conversation

satanson commented Sep 24, 2024 • edited Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

sonarqubecloud bot commented Sep 26, 2024

Quality Gate passed

github-actions bot commented Sep 26, 2024

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Sep 26, 2024

[FE Incremental Coverage Report]

file detail

github-actions bot commented Sep 26, 2024

[BE Incremental Coverage Report]

file detail

ZiheLiu Sep 27, 2024

Choose a reason for hiding this comment

ZiheLiu Sep 27, 2024

Choose a reason for hiding this comment

satanson Sep 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

satanson commented Sep 24, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading