[VL] Do not rewriteAggBufferAttributes for Complete agg mode

[Zouxxyy]

What changes are proposed in this pull request?

rewriteAggBufferAttributes is only needed by PartialMerge and Final mode

How was this patch tested?

test by the existing cases

[rui-mo]

Could you please clarify if this change fixes an existing problem? Thanks.

[Zouxxyy]

@rui-mo During our implementation of hybrid AGG (where a single AGG contains both partial-merge and complete modes), we discovered this issue:
Inside rewriteAggBufferAttributes, the code compares aggBufferAttrs with InputAttributes, which is actually incorrect in Complete mode. However, since Spark + Gluten currently does not support having both partial-merge and complete functions simultaneously, the current code path will not trigger this scenario, and it's difficult to construct a unit test (UT) for it. Nevertheless, I think we can add this defensive code.

[rui-mo]

Why would this be called for the Complete mode, as rowConstructNeeded() returns false for it?

[Zouxxyy]

Why would this be called for the Complete mode, as rowConstructNeeded() returns false for it?

As mentioned earlier, we implemented a hybrid AGG that contains both PartialMerge (or Final) and complete aggregation functions, and rowConstructNeeded is triggered as long as a PartialMerge (or Final) function is present.

It won't be triggered for the now code, but I think we can add defensive code to handle this scenario in future implementations .

[rui-mo]

rowConstructNeeded is triggered as long as a PartialMerge (or Final) function is present

Are you referring to the modified code on your side, or the current Gluten code?

As mentioned earlier, we implemented a hybrid AGG that contains both PartialMerge (or Final) and complete aggregation functions

Is there any plan to upstream this change to Gluten, or will it remain local to your side?

[Zouxxyy]

Are you referring to the modified code on your side, or the current Gluten code?

The current Gluten code, see this

  private def rowConstructNeeded(): Boolean = aggregateExpressions.exists {
    case AggregateExpression(aggFunc, PartialMerge | Final, _, _, _) =>
      aggFunc.inputAggBufferAttributes.size > 1
    case _ => false
  }

Is there any plan to upstream this change to Gluten, or will it remain local to your side?

Oh, maybe it can be launched when it becomes stable!

[rui-mo]

I understand that mixing Complete mode with other modes isn’t supported, but is there a way to add a unit test for pure Complete mode to ensure this works?

[Zouxxyy]

We already have pure complete mode case, see #7752, and it can run correctly

[jackylee-ch]

LGTM. It also works in our environments.