Copilot commented on code in PR #13144:
URL: https://github.com/apache/gluten/pull/13144#discussion_r4178591235
##########
gluten-core/src/main/scala/org/apache/gluten/iterator/IteratorsV1.scala:
##########
@@ -115,23 +115,21 @@ object IteratorsV1 {
}
}
+ // Reports each read's duration in nanoseconds. Converting each read to
milliseconds would
+ // truncate any read shorter than one millisecond to zero.
private class ReadTimeAccumulator[A](in: Iterator[A], onAdded: Long => Unit)
extends Iterator[A] {
override def hasNext: Boolean = {
val prev = System.nanoTime()
val out = in.hasNext
- val after = System.nanoTime()
- val duration = TimeUnit.NANOSECONDS.toMillis(after - prev)
- onAdded(duration)
+ onAdded(System.nanoTime() - prev)
Review Comment:
The implementation here reports raw nanoseconds directly, and the
corresponding metric is changed to `nsTiming`; it does not introduce the
`NanosToMillisAccumulator` described in the PR or preserve whole-millisecond
reporting. Please either implement the stated accumulator behavior (with its
deterministic test) or update the API/metric contract and PR description to
document the intentional unit change.
##########
gluten-core/src/test/scala/org/apache/gluten/iterator/IteratorSuite.scala:
##########
@@ -22,13 +22,45 @@ import org.apache.spark.task.TaskResources
import org.scalatest.funsuite.AnyFunSuite
+import java.util.concurrent.TimeUnit
+
class IteratorV1Suite extends IteratorSuite {
override protected def wrap[A](in: Iterator[A]): WrapperBuilder[A] =
Iterators.wrap(V1, in)
}
abstract class IteratorSuite extends AnyFunSuite {
protected def wrap[A](in: Iterator[A]): WrapperBuilder[A]
+ test("Read time is reported in nanoseconds per read") {
+ val reported = scala.collection.mutable.ArrayBuffer.empty[Long]
+ // Both hasNext and next sleep at least 1ms, so every reported duration
has a hard
+ // lower bound that does not depend on the clock's resolution.
Review Comment:
This still tests wall-clock sleeps rather than the carry-over behavior
described by the PR. A scheduler pause or GC can make a per-call
millisecond-truncating implementation report a positive duration, while the `>=
1 ms` assertions below never verify exact accumulated whole-millisecond
results. Please test the accumulator with deterministic nanosecond inputs (or
inject the clock) and assert the exact callbacks across sub-millisecond
additions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
