Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-24 Thread via GitHub 


alamb commented on PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#issuecomment-3001654371

   Thanks again @hendrikmakait and @2010YOUY01 


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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-24 Thread via GitHub 


2010YOUY01 commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2163184021


##
datafusion/physical-plan/src/unnest.rs:
##
@@ -284,7 +279,9 @@ impl UnnestStream {
 loop {
 return Poll::Ready(match ready!(self.input.poll_next_unpin(cx)) {
 Some(Ok(batch)) => {
-let timer = self.metrics.elapsed_compute.timer();
+let elapsed_compute =
+
self.metrics.baseline_metrics.elapsed_compute().clone();
+let timer = elapsed_compute.timer();

Review Comment:
   ```suggestion
   let _timer = elapsed_compute.timer(); // starts timer; 
stops on drop
   ```
   ~~nit: for readability~~ Never mind, I didn't notice the later `.done()`.



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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-24 Thread via GitHub 


alamb merged PR #16497:
URL: https://github.com/apache/datafusion/pull/16497


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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-24 Thread via GitHub 


hendrikmakait commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2164362053


##
datafusion/physical-plan/src/unnest.rs:
##
@@ -299,7 +296,7 @@ impl UnnestStream {
 continue;
 };
 self.metrics.output_batches.add(1);

Review Comment:
   Yeah, that's something I think would be good to tackle in a follow-up. I 
think the batch count might be generally useful.



##
datafusion/physical-plan/src/unnest.rs:
##
@@ -299,7 +296,7 @@ impl UnnestStream {
 continue;
 };
 self.metrics.output_batches.add(1);

Review Comment:
   Yeah, that's something I think I would be good to tackle in a follow-up. I 
think the batch count might be generally useful.



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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-24 Thread via GitHub 


2010YOUY01 commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2163184021


##
datafusion/physical-plan/src/unnest.rs:
##
@@ -284,7 +279,9 @@ impl UnnestStream {
 loop {
 return Poll::Ready(match ready!(self.input.poll_next_unpin(cx)) {
 Some(Ok(batch)) => {
-let timer = self.metrics.elapsed_compute.timer();
+let elapsed_compute =
+
self.metrics.baseline_metrics.elapsed_compute().clone();
+let timer = elapsed_compute.timer();

Review Comment:
   ```suggestion
   let _timer = elapsed_compute.timer(); // starts timer; 
stops on drop
   ```
   nit: for readability



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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-23 Thread via GitHub 


alamb commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2162564838


##
datafusion/physical-plan/src/unnest.rs:
##
@@ -299,7 +296,7 @@ impl UnnestStream {
 continue;
 };
 self.metrics.output_batches.add(1);

Review Comment:
   I verified that BaselineMetrics only records number of rows, not number of 
batches



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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-23 Thread via GitHub 


alamb commented on PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#issuecomment-2997997694

   > @alamb, sorry, my bad! I didn't notice that I didn't have the pre-commit 
hooked up and the failing `cargo fmt` that caused; CI is green now.
   
   No worries -- thank you @hendrikmakait 


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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-23 Thread via GitHub 


hendrikmakait commented on PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#issuecomment-2997890026

   @alamb, sorry, my bad! I didn't notice that I didn't have the pre-commit 
hooked up and the failing `cargo fmt` that caused; CI is green now.


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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-23 Thread via GitHub 


alamb commented on PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#issuecomment-2997814919

   Marking as draft as the CI is failing and I am trying to make it easier to 
find the PRs that need active reviews. Thank you @hendrikmakait 


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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-21 Thread via GitHub 


hendrikmakait commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2160121396


##
datafusion/physical-plan/src/metrics/baseline.rs:
##
@@ -58,6 +58,7 @@ pub struct BaselineMetrics {
 impl BaselineMetrics {
 /// Create a new BaselineMetric structure, and set `start_time` to now
 pub fn new(metrics: &ExecutionPlanMetricsSet, partition: usize) -> Self {
+// TODO: Is start_time used anywhere?

Review Comment:
   This piece of code does set the value for `start_time`. It is accessible via 
the `ExecutionPlanMetricsSet`. It still feels odd though that it is excluded 
from the `BaselineMetrics`.



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

Re: [PR] Reuse `BaselineMetrics` in `UnnestMetrics` [datafusion]

2025-06-21 Thread via GitHub 


hendrikmakait commented on code in PR #16497:
URL: https://github.com/apache/datafusion/pull/16497#discussion_r2159905884


##
datafusion/physical-plan/src/metrics/baseline.rs:
##
@@ -58,6 +58,7 @@ pub struct BaselineMetrics {
 impl BaselineMetrics {
 /// Create a new BaselineMetric structure, and set `start_time` to now
 pub fn new(metrics: &ExecutionPlanMetricsSet, partition: usize) -> Self {
+// TODO: Is start_time used anywhere?

Review Comment:
   Drive-by: Is `start_time` actually made use of? Otherwise, I suggest 
dropping it.



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