feat: Add support of TakeOrderedAndProjectExec in Comet #88
Conversation
spark/src/main/scala/org/apache/spark/sql/comet/CometTakeOrderedAndProjectExec.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/comet/CometTakeOrderedAndProjectExec.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/comet/CometTakeOrderedAndProjectExec.scala
Show resolved
Hide resolved
| if isCometNative(s.child) && isCometOperatorEnabled(conf, "takeOrderedAndProjectExec") | ||
| && isCometShuffleEnabled(conf) && | ||
| CometTakeOrderedAndProjectExec.isSupported(s.projectList, s.sortOrder, s.child) => | ||
| // TODO: support offset for Spark 3.4 |
There was a problem hiding this comment.
One minor comment: this might be a correctness issue for Spark 3.4 with offset > 0 as we simply ignore the possible offset field.
One possible way to address that would be that define a getOffset method in ShimCometSparkSessionExtensions to access offset field via reflection.
Of course it should be addressed in another PR and other xxLimitExec too.
There was a problem hiding this comment.
We can probably fallback to Spark when offset > 0?
There was a problem hiding this comment.
Yea, we need to do fallback for offset > 0.
There was a problem hiding this comment.
Yes. That's a minimal fix. However if we can access offset, we can implement the offset logic in comet as well.
There was a problem hiding this comment.
Let me do fallback in this PR. I will address offset > 0 case in follow ups.
|
I was wondering why there is test failure as I didn't see it locally. But after rebased with |
|
Found the cause. We need |
|
Merged. Thanks. |
Which issue does this PR close?
Closes #89.
Rationale for this change
TakeOrderedAndProjectExecis a common operator in Spark. In Comet, we should support it to increase query coverage.What changes are included in this PR?
How are these changes tested?