Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/22630#discussion_r223478828
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala
---
@@ -452,46 +452,68 @@ case class RangeExec(range:
org.apache.spark.sql.catalyst.plans.logical.Range)
val localIdx = ctx.freshName("localIdx")
val localEnd = ctx.freshName("localEnd")
- val range = ctx.freshName("range")
val shouldStop = if (parent.needStopCheck) {
- s"if (shouldStop()) { $number = $value + ${step}L; return; }"
+ s"if (shouldStop()) { $nextIndex = $value + ${step}L; return; }"
--- End diff --
Sorry for late comment. It would be good to discuss detail in another PR.
At first, I agree with necessary of benchmarking.
1. I think that `localIdx` can be defined as local variable outside of the
loop. Or, how about storing `localIdx` to another local variable only if
`parent.needStopCheck` is `true`.
1. Since `shouldStop()` is simply without updating, we expect the JIT
applies inlining and some optimizations.
1. If we want to call `incRecordRead`, it would be good to exit a loop
using `break` and then call `incRecordRead`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
