MaxGekk commented on code in PR #56809:
URL: https://github.com/apache/spark/pull/56809#discussion_r3485483155
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVBenchmark.scala:
##########
@@ -190,6 +190,26 @@ object CSVBenchmark extends SqlBasedBenchmark {
dates.write.option("header", true).mode("overwrite").csv(dateDir)
}
+ val timeDir = new File(path, "time").getAbsolutePath
+
+ def times = {
+ spark.range(0, rowsNum, 1, 1).mapPartitions { iter =>
+ iter.map(t => LocalTime.ofNanoOfDay((t % 86400000000L) * 1000000L))
Review Comment:
The `(t % 86400000000L) * 1000000L` formula is inconsistent and latently
unsafe:
- The modulus is micros/day (`86_400_000_000`) but the scale factor is
`*1e6`, so the two don't compose into nanos-of-day.
- Since `t < rowsNum` (1e7) is far below the modulus, the `% 86400000000L`
is a dead no-op, and the generated times only span the first ~2.8h of the day
rather than the full day.
- It would overflow `LocalTime.ofNanoOfDay` (max `86399999999999`) if
`rowsNum` is ever raised above ~8.64e7.
It doesn't fail at the current `rowsNum`, but consider aligning with
`TimeBenchmark`'s full-day wrap (`nanos % 86400000000000L`):
```suggestion
iter.map(t => LocalTime.ofNanoOfDay(t % 86400000000000L))
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala:
##########
@@ -407,6 +407,26 @@ object JsonBenchmark extends SqlBasedBenchmark {
dates.write.option("header", true).mode("overwrite").json(dateDir)
}
+ val timeDir = new File(path, "time").getAbsolutePath
+
+ def times = {
+ spark.range(0, rowsNum, 1, 1).mapPartitions { iter =>
+ iter.map(t => LocalTime.ofNanoOfDay((t % 86400000000L) * 1000000L))
Review Comment:
Same generator issue as `CSVBenchmark.scala:197`: `(t % 86400000000L) *
1000000L` has an inconsistent modulus/scale (micros/day mod, `*1e6` scale), the
`% 86400000000L` is a dead no-op at this `rowsNum`, the times don't span a full
day, and it would overflow `ofNanoOfDay` if `rowsNum` exceeded ~8.64e7. Suggest
the same full-day wrap as `TimeBenchmark`:
```suggestion
iter.map(t => LocalTime.ofNanoOfDay(t % 86400000000000L))
```
--
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]
