Filter null rows before aggregation #23052

jackychen718 · 2024-06-21T18:02:18Z

Description

Filter rows with all fileds null before aggregation.

Motivation and Context

The issue comes from #17486
Add optimiztion to drop groups/rows with all fields null

Impact

Add another opitimization rule: RemoveNullRowInAggregation.java

Test Plan

Test if any error made after applying the new optimization rule.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

== NO RELEASE NOTE ==

jaystarshot · 2024-06-21T19:02:02Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+    {
+        PlanNode source = node.getSource();
+        if (source instanceof FilterNode) {
+            return false;


Why can't the new filter added by this rule be appended to the existing filter

That was proposed to prevent the AggregationNode being optimized many times. Sure,it should be removed.

I think the check should be something like "have all is not null filters already been added to the input to the aggregation node", instead of "is the input to the aggregation node is a filter node"

jaystarshot · 2024-06-21T19:03:20Z

Can you add unit tests? Also there must be a feature flag controlling this since I am not sure if this optimization will be always effective.
What happens in count(*) case?

ClarenceThreepwood

Thanks for the contribution @jackychen718 !

This changes the sql semantics and can cause wrong results. Therefore this rule should be behind a feature flag and should definitely be disabled by default
Please add unit tests.
There are many related test failures. Please fix them before marking it ready for review

ClarenceThreepwood · 2024-06-25T03:15:28Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+    {
+        PlanNode source = node.getSource();
+        if (source instanceof FilterNode) {
+            return false;


I think the check should be something like "have all is not null filters already been added to the input to the aggregation node", instead of "is the input to the aggregation node is a filter node"

ClarenceThreepwood · 2024-06-25T03:16:04Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+        }
+        Collection<Aggregation> aggregations = node.getAggregations().values();
+        for (Aggregation aggregation : aggregations) {
+            if (aggregation.getCall().getArguments().size() > 1) {


why limit to aggregate functions of only one argument?

I see the example of the problem has only one argument. As the first attempt, I only consider one argument. I will consider arbitrary arguments.

ClarenceThreepwood · 2024-06-25T03:24:16Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+    public Result apply(AggregationNode aggregationNode, Captures captures, Context context)
+    {
+        PlanNode source = aggregationNode.getSource();
+        if (context.getLookup().resolveGroup(source).findFirst().filter(node -> node instanceof FilterNode)


This is not necessary since you already check for this in the pattern detection

Yes. I will modify the pattern detection part later on.

ClarenceThreepwood · 2024-06-25T03:28:36Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+
+        RowExpression andForm = new SpecialFormExpression(AND, BOOLEAN, ImmutableList.copyOf(nullFormList));
+        RowExpression notCallPredicate = new CallExpression("not",
+                new FunctionResolution(createTestFunctionAndTypeManager().getFunctionAndTypeResolver()).notFunction(),


Do not use createTestFunctionAndTypeManager. Get it from the optimizer metadata. See other optimizer rules for examples

jackychen718 · 2024-06-26T02:38:06Z

Can you add unit tests? Also there must be a feature flag controlling this since I am not sure if this optimization will be always effective. What happens in count(*) case?

I will add unit tests. Currently, I did not consider count(*) as a special way. From your reminding, I do not think I can still add filter before aggregation in this scenario. I will exclude this scenario in the pattern match process. Is it ok? @jaystarshot

ZacBlanco · 2024-06-27T19:33:16Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveNullRowInAggregation.java

+            nullFormList.add(nullForm);
+        }
+
+        RowExpression andForm = new SpecialFormExpression(AND, BOOLEAN, ImmutableList.copyOf(nullFormList));


nit: I would use ImmutableList.builder() for nullFormList, call add in the loop, and then pass nullFormList.build() when generating the andForm expression. It prevents copying the list.

jackychen718 requested review from jaystarshot, feilong-liu and a team as code owners June 21, 2024 18:02

jackychen718 requested a review from presto-oss June 21, 2024 18:02

jaystarshot reviewed Jun 21, 2024

View reviewed changes

tdcmeehan self-assigned this Jun 23, 2024

ClarenceThreepwood self-requested a review June 24, 2024 22:16

ClarenceThreepwood requested changes Jun 25, 2024

View reviewed changes

jackychen718 force-pushed the Drop-Agg-NULL branch from e647d6f to fd4f28a Compare June 27, 2024 18:59

ZacBlanco reviewed Jun 27, 2024

View reviewed changes

Filter null rows before aggregation

86538ad

jackychen718 force-pushed the Drop-Agg-NULL branch from fd4f28a to 86538ad Compare June 28, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter null rows before aggregation #23052

Filter null rows before aggregation #23052

jackychen718 commented Jun 21, 2024 •

edited

Loading

jaystarshot Jun 21, 2024

jackychen718 Jun 21, 2024

ClarenceThreepwood Jun 25, 2024

jaystarshot commented Jun 21, 2024

ClarenceThreepwood left a comment

ClarenceThreepwood Jun 25, 2024

ClarenceThreepwood Jun 25, 2024

jackychen718 Jun 26, 2024

ClarenceThreepwood Jun 25, 2024

jackychen718 Jun 26, 2024

ClarenceThreepwood Jun 25, 2024

jackychen718 Jun 26, 2024

jackychen718 commented Jun 26, 2024 •

edited

Loading

ZacBlanco Jun 27, 2024

jackychen718 Jun 28, 2024

Filter null rows before aggregation #23052

Are you sure you want to change the base?

Filter null rows before aggregation #23052

Conversation

jackychen718 commented Jun 21, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaystarshot commented Jun 21, 2024

ClarenceThreepwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackychen718 commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackychen718 commented Jun 21, 2024 •

edited

Loading

jackychen718 commented Jun 26, 2024 •

edited

Loading