venkata91 opened a new pull request, #17562:
URL: https://github.com/apache/iceberg/pull/17562

   Iceberg reports scan **bytes** and file/manifest **counts** per task, but 
the only timer in the metrics system is `totalPlanningDuration` 
(`ScanMetrics.java`), which is started in `SnapshotScan` around driver-side 
planning. `CommitMetrics.totalDuration` is likewise commit, not data movement.
   
   So there is no metric for how long a task spent actually reading, and scan 
throughput cannot be derived from Iceberg metrics alone. That gap shows up when 
comparing the same table read from two clusters (e.g. checking whether a read 
is crossing regions): the bytes are there, the elapsed time is not, and task 
duration is dominated by whatever the query does after the scan.
   
   ## Change
   
   Add `scanDuration`: wall time a task spends inside `BaseReader.next()`, 
which covers opening each file split and pulling rows/batches from it.
   
   - Timed per `next()` call, not per row. Spark makes the same tradeoff for 
its own `scanTime` (`DataSourceScanExec`), where the comment notes a per-row 
timer costs more than the read it measures.
   - Accumulated in a `finally` so a task that throws mid-scan still reports 
what it read.
   - Emitted as `TaskScanDuration` from `BatchDataReader` and `RowDataReader`, 
aggregated by `ScanDuration`, mirroring the existing 
`TaskTotalPlanningDuration` / `TotalPlanningDuration` pair, so it lands on the 
Spark UI scan node next to `totalPlanningDuration`.
   
   Reported in milliseconds, matching `totalPlanningDuration`.
   
   ## Testing
   
   `TestSparkReadMetrics` asserts `scanDuration` is present and non-negative in 
all three existing cases (V1, V2, V3 tables). Ran 
`:iceberg-spark:iceberg-spark-4.1_2.13:test --tests "*TestSparkReadMetrics*"` 
-> 3 tests, 0 failures. `spotlessCheck` and `checkstyleMain` pass.
   
   ## Notes
   
   - Only `spark/v4.1` here to keep the change reviewable. Happy to backport to 
4.0/3.5 in this PR or a follow-up, whichever reviewers prefer.
   - A non-negative assertion is used rather than `> 0`, since a fully cached 
small scan can complete inside a millisecond.


-- 
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]

Reply via email to