*: only add default value for final aggregation to fix the aggregate push down (partition) union case #35443

tiancaiamao · 2022-06-16T08:03:26Z

What problem does this PR solve?

Issue Number: close #35295

Problem Summary:

When table t is empty, this get empty result:

select avg(id) from t group by id;

While this get NULL:

select avg(id) from t;

We add NULL (default value) to the empty result here:

tidb/executor/aggregate.go

Lines 1368 to 1379 in 2de01b4

    
           if e.childResult.NumRows() == 0 { 
        
           	if !e.isChildReturnEmpty { 
        
           		err = e.appendResult2Chunk(chk) 
        
           	} else if e.defaultVal != nil { 
        
           		chk.Append(e.defaultVal, 0, 1) 
        
           	} 
        
           	e.executed = true 
        
           	return err 
        
           } 
        
           // Reach here, "e.childrenResults[0].NumRows() > 0" is guaranteed. 
        
           e.isChildReturnEmpty = false 
        
           e.inputRow = e.inputIter.Begin()

Whether or not add the NULL row depends on some conditions.

Note, for this case, the added NULL row make the final result wrong:

What is changed and how it works?

Only add NULL for final aggregation.
Otherwise the add NULL row operation would be done several times and send a wrong NULL row to the final aggregation.

In aggregation push down optimze rule, I'll set the pushed down aggregation to non-final
In physical plan, aggregation push down to cop phase, I'll not blindly change the upper aggregation to final/complete... because it's parent might be final and it's a partial itself
In the executor builder phase, only set the default value for final aggregation

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

…push down (partition) union case

ti-chi-bot · 2022-06-16T08:03:27Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

qw4990
winoros

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

tiancaiamao · 2022-06-17T15:17:57Z

/run-unit-test

tiancaiamao · 2022-06-20T04:47:08Z

/run-unit-test

sre-bot · 2022-06-20T05:32:49Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/e799be1077d7efc8c499f0315159a4f81eb06bf7

tiancaiamao · 2022-06-20T05:39:46Z

PTAL @XuHuaiyu @qw4990

XuHuaiyu · 2022-06-20T06:06:04Z

expression/aggregation/descriptor.go

@@ -123,8 +123,6 @@ func (a *AggFuncDesc) Split(ordinal []int) (partialAggDesc, finalAggDesc *AggFun
 		partialAggDesc.Mode = Partial1Mode
 	} else if a.Mode == FinalMode {
 		partialAggDesc.Mode = Partial2Mode
-	} else {


Why remove this check?

In the previous assumption, there will be no partial mode agg before coprocessor push down...

But now I change the logical plan aggregation to partial, so in the coprocessor push down here, this line may meet partial mode agg.

planner/core/physical_plans.go

XuHuaiyu · 2022-06-20T06:21:35Z

planner/core/task.go

@@ -1417,7 +1421,11 @@ func BuildFinalModeAggregation(
 				}
 			}

-			finalAggFunc.Mode = aggregation.FinalMode
+			if aggFunc.Mode == aggregation.CompleteMode {


I do not understand what does this code block mean

In the old assumption, before the coprocessor push down, the agg mode is either Complete or Final. So after the push down, the parent agg become Final mode.

But in the new assumption, before the coprocessor push down, the agg mode can be partial. So here the parent agg mode is set to different value accordingly.

For final before, we set it to final.
For partial, it's set to partial2

winoros · 2022-06-22T16:47:52Z

planner/core/task.go

@@ -1417,7 +1421,11 @@ func BuildFinalModeAggregation(
 				}
 			}

-			finalAggFunc.Mode = aggregation.FinalMode
+			if aggFunc.Mode == aggregation.CompleteMode {


I think you mean that when we are in the partition union mode. The Agg->PartitionUnion->TableReader may become Agg->PartitionUnion->Agg->TableReader, then we do the push down. So we need to add a new if-condition check.

You can add some comments here.

Agg1->PartitionUnion->Agg2->TableReader

In coprocessor push down, Agg2->TableReader become Agg3(root)->TableReader->Agg4(cop)->TableScan(cop), and the final plan become Agg1->PartitionUnion->Agg3(root)->TableReader->Agg4(cop)->TableScan(cop)

In the past, Agg2 is always Complete or Final, but in this PR here Agg2 could be Partial
So BuildFinalModeAggregation need to consider the difference here.

I add some comment in the code so the reviewer know what happen.