jackylee-ch commented on code in PR #13144:
URL: https://github.com/apache/gluten/pull/13144#discussion_r4175404191
##########
gluten-core/src/test/scala/org/apache/gluten/iterator/IteratorSuite.scala:
##########
@@ -24,6 +24,21 @@ import org.scalatest.funsuite.AnyFunSuite
class IteratorV1Suite extends IteratorSuite {
override protected def wrap[A](in: Iterator[A]): WrapperBuilder[A] =
Iterators.wrap(V1, in)
+
+ test("Sub-millisecond read durations accumulate with carry-over instead of
truncating") {
+ val reported = scala.collection.mutable.ArrayBuffer.empty[Long]
+ val accumulator = new IteratorsV1.NanosToMillisAccumulator(reported += _)
Review Comment:
This asserts `NanosToMillisAccumulator`'s arithmetic directly but never
drives `ReadTimeAccumulator` / `collectReadMillis`, so a regression in the
actual `selfTime` wiring (e.g. a dropped `accumulator.add`) would keep this
test green. Could it assert the real path instead — e.g. by injecting a
nano-clock into `ReadTimeAccumulator` so the metric it feeds can be checked
deterministically?
##########
gluten-core/src/main/scala/org/apache/gluten/iterator/IteratorsV1.scala:
##########
@@ -115,23 +115,37 @@ object IteratorsV1 {
}
}
+ // Accumulates nanosecond read durations and reports only whole
milliseconds, carrying the
+ // sub-millisecond remainder to the next call so reads shorter than a
millisecond are not
+ // truncated to zero. Package-visible so the carry-over can be tested
deterministically
+ // without depending on wall-clock timing.
+ private[iterator] class NanosToMillisAccumulator(onAdded: Long => Unit) {
Review Comment:
The only caller of `collectReadMillis` is `ColumnarToColumnarExec.selfTime`
— the "time to convert batches" UI timing metric on a columnar-to-columnar
node. Is there a production case where this metric reading ~0 actually misled
someone? Adding a stateful per-batch accumulator for a cosmetic metric is hard
to justify without one. And `selfTime` is a coarse millisecond metric while the
scan-side timings are nanos — is `ms` even the right unit here, rather than
fixing the layer?
--
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]
