UNPICK review PR_REVIEW_01.md 2610061732

Author: kosiew

PR_REVIEW_01.md

@@ -10,20 +10,26 @@ author: lyne7-sc
 - Cached filter-column projection and original-row mappings preserve output payloads and spill bitmap coordinates; Cartesian filter evaluation is extracted into a shared helper.
 - Added bitmap/early-stop unit tests, compaction and spill/replay SQLLogicTests, and six SQL benchmarks covering no matches, staged matches, repeated matches, and final-probe copying.
 
-Scope: cumulative PR changes in `b9fb06973b^..9d19d30a38`, accounting for both main merges. Reviewed final PR delta against `9d19d30a38^2`; upstream-only changes excluded. No UNPICK commits reviewed. Checkout differs from reviewed head only in agent guidance. Existing Codecov comment not repeated.
+Scope: cumulative PR changes in `b9fb06973b^..9d19d30a38`, accounting for both main merges. Reviewed final PR delta against `9d19d30a38^2`; upstream-only changes excluded. No UNPICK commits reviewed. Checkout differs from reviewed head only in agent guidance and this review file. GitHub PR head still matches `9d19d30a38`. Existing Codecov comment not repeated.
 
 ## Decision
 - [x] Approve with suggestions
 - [ ] Request changes
 
 The issue is valid: after a right row matches, subsequent comparisons cannot change semi/anti membership. Traced candidate deduplication, sliced bitmap segments, compaction mappings, empty-input handling, per-right-batch reset, and original-coordinate spill merging. No public API or schema change found.
 
-Validation:
+Validation recorded by the original review (not rerun during this recheck):
 - `cargo test -p datafusion-physical-plan joins::nested_loop_join::tests --lib --offline`: **77 passed**.
 - `cargo test -p datafusion-sqllogictest --test sqllogictests --offline -- nested_loop_join_existence nested_loop_join_spill`: **both files passed**. macOS linker emitted an unwind-table-size warning.
 - `git diff --check 9d19d30a38^2 9d19d30a38`: passed.
 - `target/debug/benchmark_runner nlj --query 20 --dry-run`: configuration resolved successfully; this does not execute or validate the physical plan.
-- Full workspace CI and before/after performance measurements were not run; no measured speedup claimed by this review.
+- Full workspace CI and before/after performance measurements were not run locally; no independently measured speedup claimed by this review.
+
+Recheck evidence:
+- `RightExistenceProbe::new` (`nested_loop_join.rs:1963–1980`) deduplicates right source indices in encounter order and remaps filter indices to that projection. `compact` (`2091–2114`) preserves original row coordinates and applies the same row selection to every projected column.
+- Existing staged-compaction SQL cases (`nested_loop_join_existence.slt:48–61`) reference only `r.k`; `prepare_join_filter` (`nested_loop_join.rs:4905–4916`) likewise supplies only one right filter column. The proposed multi-column combination is not redundant.
+- PR description now reports before/after benchmarks, including small regressions for repeated-match (+1.84%) and final-probe (+1.28%) controls. These author-reported results were not reproduced here.
+- GitHub CI at recheck: no failed checks; several checks, including amd64 tests, clippy, and benchmark verification, remain pending. Approval does not substitute for required CI.
 
 ## Blocking findings
 No blocking findings.
@@ -32,11 +38,11 @@ No blocking findings.
 1. file: datafusion/physical-plan/src/joins/nested_loop_join.rs
    line: 1967
    side: RIGHT
-   body: Low — Consider a small regression test whose filter references two distinct right columns in a different order from their source schema, with staged matches forcing compaction. The new projection/remapping loop owns this positional contract, while the added SQL compaction cases reference only one right filter column. Assert original payloads for both RightSemi and RightAnti. Non-blocking: the remapping logic looks correct and existing tests pass.
+   body: Low — Consider a small SQLLogicTest with two distinct right filter columns and staged matches forcing compaction. Make each column affect membership and assert original payloads for both RightSemi and RightAnti. If testing reversed projection order, confirm the physical JoinFilter references those columns out of source-schema order; SQL expression order alone does not prove this. The new projection/remapping loop owns this positional contract, while existing staged-compaction cases reference only one right filter column. Non-blocking coverage improvement, not a demonstrated bug: the remapping and common row selection look correct.
 
 ## High-impact refactor opportunities (out of scope)
 No high-impact opportunity justified in touched code. The introduced probe state already owns candidate coordinates, original coordinates, and match deduplication together; the shared filter helper removes duplicated evaluation. Splitting these further would add interfaces without resolving an observed correctness or maintenance problem. The pre-existing stream/spill state machine is larger, but this PR does not establish evidence for a broad redesign.
 
 ## Follow-up actions
 - Optionally add the multi-column remapping regression above.
-- Confirm normal CI passes and compare the new benchmark cases against main using identical release settings, especially no-match and repeated-match controls.
+- Confirm remaining required CI passes. If reproducing the author-reported benchmarks, compare against main using identical release settings, especially no-match and repeated-match controls.