Re: [PR] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
alamb merged PR #16675: URL: https://github.com/apache/datafusion/pull/16675 -- 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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
Standing-Man commented on code in PR #16675:
URL: https://github.com/apache/datafusion/pull/16675#discussion_r2186251681
##
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##
@@ -2032,7 +2034,10 @@ impl SortMergeJoinStream {
let record_batch =
concat_batches(&self.schema,
&self.staging_output_record_batches.batches)?;
self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(record_batch.num_rows());
+let _ = self
+.join_metrics
+.baseline_metrics
+.record_poll(Poll::Ready(Some(Ok(record_batch.clone();
Review Comment:
you're right, `record_poll` involves an expensive clone, while
`record_output` can achieve the same result more efficiently.
--
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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
comphead commented on code in PR #16675:
URL: https://github.com/apache/datafusion/pull/16675#discussion_r2185865853
##
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##
@@ -2032,7 +2034,10 @@ impl SortMergeJoinStream {
let record_batch =
concat_batches(&self.schema,
&self.staging_output_record_batches.batches)?;
self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(record_batch.num_rows());
+let _ = self
+.join_metrics
+.baseline_metrics
+.record_poll(Poll::Ready(Some(Ok(record_batch.clone();
Review Comment:
Can we just `record_output(num_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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
comphead commented on code in PR #16675:
URL: https://github.com/apache/datafusion/pull/16675#discussion_r2185864775
##
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##
@@ -2032,7 +2034,10 @@ impl SortMergeJoinStream {
let record_batch =
concat_batches(&self.schema,
&self.staging_output_record_batches.batches)?;
self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(record_batch.num_rows());
+let _ = self
+.join_metrics
+.baseline_metrics
+.record_poll(Poll::Ready(Some(Ok(record_batch.clone();
Review Comment:
I would think cloning and returning an entire `record_batch` would be
expensive, than just a single i64 that is `num_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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
Standing-Man commented on code in PR #16675:
URL: https://github.com/apache/datafusion/pull/16675#discussion_r2184734711
##
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##
@@ -2032,7 +2034,10 @@ impl SortMergeJoinStream {
let record_batch =
concat_batches(&self.schema,
&self.staging_output_record_batches.batches)?;
self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(record_batch.num_rows());
+self.join_metrics
+.baseline_metrics
+.output_rows()
+.add(record_batch.num_rows());
Review Comment:
Thanks for the suggestion! I've updated the code accordingly — could you
please take a look and confirm if the change addresses your comment?
--
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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
2010YOUY01 commented on code in PR #16675:
URL: https://github.com/apache/datafusion/pull/16675#discussion_r2184636583
##
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##
@@ -2032,7 +2034,10 @@ impl SortMergeJoinStream {
let record_batch =
concat_batches(&self.schema,
&self.staging_output_record_batches.batches)?;
self.join_metrics.output_batches.add(1);
-self.join_metrics.output_rows.add(record_batch.num_rows());
+self.join_metrics
+.baseline_metrics
+.output_rows()
+.add(record_batch.num_rows());
Review Comment:
I have a suggestion, see:
https://github.com/apache/datafusion/pull/16674/files#r2184635238
--
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] Refactor SortMergeJoinMetrics to reuse BaselineMetrics [datafusion]
jatin510 commented on PR #16675: URL: https://github.com/apache/datafusion/pull/16675#issuecomment-3034553196 lgtm -- 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]
