mbutrovich commented on code in PR #2521:
URL: https://github.com/apache/iceberg-rust/pull/2521#discussion_r3326721428
##########
crates/integrations/datafusion/src/physical_plan/scan.rs:
##########
@@ -237,6 +260,19 @@ async fn get_batch_stream(
Ok(Box::pin(stream))
}
+fn stream_with_baseline_metrics(
+ mut stream: Pin<Box<dyn Stream<Item = DFResult<RecordBatch>> + Send>>,
+ baseline_metrics: BaselineMetrics,
+) -> Pin<Box<dyn Stream<Item = DFResult<RecordBatch>> + Send>> {
+ futures::stream::poll_fn(move |cx| {
+ let baseline_metrics = baseline_metrics.clone();
Review Comment:
I don't think `baseline_metrics.clone()` is needed.
`BaselineMetrics::elapsed_compute()` and `record_poll()` both take `&self`, so
the captured value can be used directly. The clone is cheap (Arc/Count copies)
but it's dead weight on every poll and reads as if there's a borrow problem to
work around. Suggest:
```rust
futures::stream::poll_fn(move |cx| {
let _timer = baseline_metrics.elapsed_compute().timer();
let poll = stream.as_mut().poll_next(cx);
baseline_metrics.record_poll(poll)
})
```
--
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]
