Re: [PR] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-07 Thread via GitHub 


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

   Thanks @Samyak2 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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-07 Thread via GitHub 


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


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-06 Thread via GitHub 


Samyak2 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3041233637

   Fixed the formatting issues


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-05 Thread via GitHub 


Samyak2 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3040935741

   > > Actually, I see the same behavior on latest main. Looks like the 
output_rows metric in nested loop join is currently wrong?
   > 
   > Thank you for the catch! Here might also need `record_poll()`: 
https://github.com/apache/datafusion/pull/16500/files#r2187952197
   
   Thanks for the pointer! It makes sense now. I have added a record_poll there 
and also updated the `output_batches` metric inside the function. Please take a 
look


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-05 Thread via GitHub 


2010YOUY01 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3040715483

   > Actually, I see the same behavior on latest main. Looks like the 
output_rows metric in nested loop join is currently wrong?
   
   Thank you for the catch! Here might also need `record_poll()`: 
https://github.com/apache/datafusion/pull/16500/files#r2187952197


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-05 Thread via GitHub 


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


##
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##
@@ -825,7 +825,8 @@ impl NestedLoopJoinStream {
 handle_state!(ready!(self.fetch_probe_batch(cx)))
 }
 NestedLoopJoinStreamState::ProcessProbeBatch(_) => {
-handle_state!(self.process_probe_batch())
+let poll = handle_state!(self.process_probe_batch());
+self.join_metrics.baseline.record_poll(poll)
 }
 NestedLoopJoinStreamState::ExhaustedProbeSide => {

Review Comment:
   I think this state is also possible to output final result: because if it's 
left join, NLJ has to exhaust right input first, then output the unmatched left 
rows.



-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-05 Thread via GitHub 


Samyak2 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3039652143

   Actually, I see the same behavior on latest main. Looks like the output_rows 
metric in nested loop join is currently wrong?


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-07-05 Thread via GitHub 


Samyak2 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3039543288

   I have added asserts for metrics in the existing join tests. The ones in 
hash_join and cross_join are working. The asserts in nested_loop_join are 
currently failing due to a mismatch in `output_rows`. I'm debugging this, but I 
have put this out here in case anyone has an idea about this (or if this is 
expected):
   ```
   failures:
   
    joins::nested_loop_join::tests::join_left_anti_with_filter stdout 
   
   thread 'joins::nested_loop_join::tests::join_left_anti_with_filter' panicked 
at datafusion/physical-plan/src/joins/nested_loop_join.rs:1405:9:
   assertion `left == right` failed
 left: 0
right: 2
   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
   
    joins::nested_loop_join::tests::join_left_semi_with_filter stdout 
   
   thread 'joins::nested_loop_join::tests::join_left_semi_with_filter' panicked 
at datafusion/physical-plan/src/joins/nested_loop_join.rs:1375:9:
   assertion `left == right` failed
 left: 0
right: 1
   
    joins::nested_loop_join::tests::join_left_with_filter stdout 
   
   thread 'joins::nested_loop_join::tests::join_left_with_filter' panicked at 
datafusion/physical-plan/src/joins/nested_loop_join.rs:1282:9:
   assertion `left == right` failed
 left: 1
right: 3
   
    joins::nested_loop_join::tests::join_full_with_filter stdout 
   
   thread 'joins::nested_loop_join::tests::join_full_with_filter' panicked at 
datafusion/physical-plan/src/joins/nested_loop_join.rs:1346:9:
   assertion `left == right` failed
 left: 3
right: 5
   
    joins::nested_loop_join::tests::join_left_mark_with_filter stdout 
   
   thread 'joins::nested_loop_join::tests::join_left_mark_with_filter' panicked 
at datafusion/physical-plan/src/joins/nested_loop_join.rs:1495:9:
   assertion `left == right` failed
 left: 0
right: 3
   ```


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-06-28 Thread via GitHub 


2010YOUY01 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3016318383

   Thank you! this implementation looks correct to me.
   
   Since the state transition in joins are tricky, could you add a test (or 
ensure there are some existing tests), to double-check this refactor won't 
change the result of the related metrics?


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-06-28 Thread via GitHub 


Samyak2 commented on PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#issuecomment-3015164616

   Rebased on latest main


-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-06-24 Thread via GitHub 


Samyak2 commented on code in PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#discussion_r2164580513


##
datafusion/physical-plan/src/joins/cross_join.rs:
##
@@ -632,7 +632,7 @@ impl CrossJoinStream {
 }
 
 self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(batch.num_rows());
+self.join_metrics.baseline.record_output(batch.num_rows());

Review Comment:
   I have updated it to use `record_poll`. Could you check if that's what you 
were looking for? It's my first time with this codebase, so I'm a little unsure 
if I did the right thing



-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-06-24 Thread via GitHub 


Samyak2 commented on code in PR #16500:
URL: https://github.com/apache/datafusion/pull/16500#discussion_r2164583753


##
datafusion/physical-plan/src/joins/utils.rs:
##
@@ -1212,12 +1213,19 @@ pub(crate) struct BuildProbeJoinMetrics {
 pub(crate) input_rows: metrics::Count,
 /// Number of batches produced by this operator
 pub(crate) output_batches: metrics::Count,
-/// Number of rows produced by this operator
-pub(crate) output_rows: metrics::Count,
+}
+
+impl Drop for BuildProbeJoinMetrics {

Review Comment:
   I agree. This is not intuitive at all. When I came back to this PR to act on 
your suggestions, it took me a while to remember how this works :)
   
   I have added a comment for now. I would be open to other ways of handling 
this!



-- 
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] chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` [datafusion]

2025-06-24 Thread via GitHub 


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


##
datafusion/physical-plan/src/joins/cross_join.rs:
##
@@ -632,7 +632,7 @@ impl CrossJoinStream {
 }
 
 self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(batch.num_rows());
+self.join_metrics.baseline.record_output(batch.num_rows());

Review Comment:
   I suggest to use 
https://github.com/apache/datafusion/blob/c4d2cd720724aa1b9c0e819a015f4a0d8ba18cd7/datafusion/physical-plan/src/metrics/baseline.rs#L124
   to keep the implementation consistent with other operators, and the same for 
other join operators in this PR.



##
datafusion/physical-plan/src/joins/utils.rs:
##
@@ -1212,12 +1213,19 @@ pub(crate) struct BuildProbeJoinMetrics {
 pub(crate) input_rows: metrics::Count,
 /// Number of batches produced by this operator
 pub(crate) output_batches: metrics::Count,
-/// Number of rows produced by this operator
-pub(crate) output_rows: metrics::Count,
+}
+
+impl Drop for BuildProbeJoinMetrics {

Review Comment:
   I don't think it's the best way but I guess it's okay to merge 🤔 If we 
forget to count a specific time period in some join operator, we can fix it in 
the future.
   
   Could you also add a brief comment to explain this drop?



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