chore: refactor MaterializingSortMergeJoinStream into generators and simplify code to be textbook like as possible - #23976
Conversation
|
run benchmark smj |
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (ccdd363) to 68d5874 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (ccdd363) to 68d5874 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (ccdd363) to 68d5874 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (ccdd363) to 68d5874 (merge-base) diff using: smj File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesmj — base (merge-base)
smj — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23976 +/- ##
==========================================
+ Coverage 80.75% 80.84% +0.08%
==========================================
Files 1096 1096
Lines 373282 373902 +620
Branches 373282 373902 +620
==========================================
+ Hits 301440 302274 +834
+ Misses 53869 53575 -294
- Partials 17973 18053 +80 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
run benchmark smj |
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (5bab8f5) to 68d5874 (merge-base) diff using: smj File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (5bab8f5) to 68d5874 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (5bab8f5) to 68d5874 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (5bab8f5) to 68d5874 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesmj — base (merge-base)
smj — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
run benchmark smj |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (5bab8f5) to 68d5874 (merge-base) diff using: smj File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesmj — base (merge-base)
smj — branch
File an issue against this benchmark runner |
|
run benchmark smj |
MaterializingSortMergeJoinStream into generatorsMaterializingSortMergeJoinStream into generators and simplify code to be textbook like as possible
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (cb881c7) to 68d5874 (merge-base) diff using: smj File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesmj — base (merge-base)
smj — branch
File an issue against this benchmark runner |
|
run benchmark tpch10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing refactor-materializing-smj (cb881c7) to 68d5874 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
|
||
| match ready!(self.spill_stream.as_mut().unwrap().poll_next_unpin(cx)) { | ||
| Some(Ok(batch)) => { | ||
| match spill_stream.next().await.transpose()? { |
There was a problem hiding this comment.
restore_spilled_batches awaits the spill stream here without stop_join_time()/start_join_time() around it, per the stop_join_time doc comment: "The join's own spill write and read-back are NOT excluded — that time is join work." That's a real change from the old poll_next, which returned out via ready!(self.poll_spilled_batches(cx, ...)) on a genuine Poll::Pending, stopping _timer for that wait.
This is the same join_time semantics change #23761 required calling out before merge (that PR's description was updated to note the metric now includes spill read-back time). This PR's description currently says "Are there any user-facing changes? nope" — worth adding the same note here, since join_time is visible in EXPLAIN ANALYZE.
Which issue does this PR close?
Part of:
Rationale for this change
Simplify the code and make code similar to textbook so it is easier to read
What changes are included in this PR?
Changed to async generators + simplified to be similar to textbook
Most of the code was written by Claude Fable 5 since it was just too large to comprehend in head
Are these changes tested?
existing tests + a little more tests to verify we are not counting the childs in the join time
Are there any user-facing changes?
The join_time now includes the time to read from the async spill stream between pending which is arguable more correct since this time is part of the operator, although long waits between pending calls will be counted in the op
join_timewhile the alternative is not counting the read from file and decoding...