[FEA] Audit WindowExec

[nartal1]

Is your feature request related to a problem? Please describe.

• Ensure Spark and Rapids plugin version of the exec match functionality.

• Verify Config specific to the operator match.

• Verify API is consistent and fully translated.

• Port relevant tests.

[nartal1]

There is difference in API. Will sync up with @mythrocks to understand the missing parameters.

   case class WindowExec(
    windowExpression: Seq[NamedExpression],
    partitionSpec: Seq[Expression],
    orderSpec: Seq[SortOrder],
    child: SparkPlan)

These configs are in the tests of Spark test Suite-
Not required:
SQLConf.OPTIMIZER_EXCLUDED_RULES
SQLConf.WINDOW_EXEC_BUFFER_IN_MEMORY_THRESHOLD

[mythrocks]

Apologies for the delay in responding on this issue.

The reason for this variance is that these parameters were redundant. The partitionSpec, orderSpec, etc. were available from the Apache Spark WindowExpression class, via the WindowSpecDefinition. It is therefore available in GpuWindowExpressions, where the evaluation happens. I found that in GpuWindowExec was dropping those expressions on the floor, and extracting them in GpuWindowExpression.

Examining the code again, I see that Apache Spark uses it in the definition of WindowExec::requiredChildOrdering(). It doesn't seem to affect the execution of WindowExec, since this information is available in GpuWindowExpression.

@tgravescs, I'm not too familiar with requiredChildOrdering(). Would it be alright to ignore this API variance for now, until such a time as when the output column ordering is affected?

[nartal1]

Thanks @mythrocks for detailed explanation. I did not know that the information is available from WindowExpression.

[tgravescs]

so I think the default requiredChildOrdering is going to be empty and the when Spark goes to make sure its requirements are fullfilled will always be true. It means that if the ordering got messed with at all during shuffle it won't fix it. I'm guessing that is never happening right now and I'm not sure off the top of my head a condition that would cause that.
I think it would be good for us to at least file an issue to investigate it further. If we can add requiredChildOrdering back I think it would be good in case it does hit some case. The only issue might be if databricks is different.

override def requiredChildOrdering: Seq[Seq[SortOrder]] =
Seq(partitionSpec.map(SortOrder(_, Ascending)) ++ orderSpec)

[mythrocks]

If we can add requiredChildOrdering back I think it would be good in case it does hit some case.

I have filed #486. I'll try add this back in the next sprint.

[nartal1]

Thanks @mythrocks and @tgravescs . I am closing this as other things are audited and follow on issues are filed.