fix: Native window operator should be CometUnaryExec#774
Conversation
| this.serializedPlanOpt == other.serializedPlanOpt | ||
| case _ => | ||
| false | ||
| } |
There was a problem hiding this comment.
This native plan is incorrect. This uses the window operator's output as the pseudo scan's output, but it should be the child plan's output. The incorrect output causes errors in DataFusion Window operator.
There was a problem hiding this comment.
Besides, native window operator requires correct partitioning and ordering information from its child operator. The current native scan operator doesn't have these information. That also causes DataFusion Window operator cannot produce correct ordering partition by info.
| } | ||
|
|
||
| object CometWindowExec { | ||
| def getNativePlan( |
There was a problem hiding this comment.
Because Window is native operator, we should chain it in the native operators instead of creating a pseudo scan for it.
There was a problem hiding this comment.
It also can improve performance as we don't need additional native/JVM bridge.
huaxingao
left a comment
huaxingao
left a comment
There was a problem hiding this comment.
Thanks for catching the problem!
|
Thanks @huaxingao |
2 participants
Which issue does this PR close?
Closes #775.
Rationale for this change
What changes are included in this PR?
How are these changes tested?