Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
==========================================
+ Coverage 66.68% 68.72% +2.03%
==========================================
Files 186 195 +9
Lines 66795 67835 +1040
Branches 9507 9668 +161
==========================================
+ Hits 44543 46617 +2074
+ Misses 19572 18365 -1207
- Partials 2680 2853 +173 |
scott-routledge2
left a comment
scott-routledge2
left a comment
There was a problem hiding this comment.
LGTM, thanks @DrTodd13
| // Write batches to file in pipeline order. | ||
| // Wait for last write pipeline batch to finish before running | ||
| // this one. | ||
| prev_batch_se->event.wait(se->stream); |
There was a problem hiding this comment.
aren't we already waiting on each batch as it comes through inside the base operator? Do we have to change that part?
PhysicalGPUSink::ConsumeBatch
There was a problem hiding this comment.
You are referencing waiting on the previous stage of the pipeline for the same batch. This code segment is waiting on the write parquet of the previous batch to finish. I've seen conflicting information on whether this is strictly necessary but it can't hurt. Just want to double make sure that somehow things don't get written to the file in the wrong order.
Changes included in this PR
Testing strategy
Run locally with async turned on.
run_ci
User facing changes
None.
Checklist
[run CI]in your commit message.