Repository navigation
exec: decode build rows of either layout in the reversed grace join - #253
Conversation
A reversed semi or anti join with a residual condition decoded its build rows in mark_pairs through the arrow-row payload accessor, which returns an empty slice for chunks stored in the row layout, so TPC-H q21 with krishiv.exec.join.rowLayout=true panicked while spilling (index out of bounds on a zero-length row). Candidate pairs now keep the build row index and mark_pairs decodes through Shared::build_columns, the same path the probe uses, so either payload format works. A new test runs a reversed semi join with a condition in both layouts. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Indexed residual evaluation and layout coverage crates/krishiv-exec/src/join/grace.rs |
Candidate pairs now retain probe and build row indices. Residual evaluation uses the current RowArena and Shared::build_columns. The test checks that both row layouts emit the expected odd-key probe rows. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related PRs
- KrishivAI/krishiv-engine#237: Adds existence joins and residual-condition evaluation across hash, grace-spill, and row-layout paths.
Merge Risk: 
dd3cc
-
The row-layout fix has no established outstanding merge-blocking issue; it is ready for normal checks.
Architecture risk: 
dd3cc
-
The change affects 1 system.
Changed systems:
cratesArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — crates (service) was modified; 1 changed file maps to changed impact.
Before / after behavior
- observed — Modified behavior in crates/krishiv-exec/src/join/grace.rs: Candidate pairs now store probe and build row indices for the current arena chunk instead of probe indices and borrowed payload bytes.
- observed — Modified behavior in crates/krishiv-exec/src/join/grace.rs: When a residual filter exists, matching candidates are queued by row index; batches reaching
PAIR_BATCHand the final remainder are evaluated using the probe batch and current arena. Without a filter, matching probe rows are still marked directly. - observed — Modified behavior in crates/krishiv-exec/src/join/grace.rs:
mark_pairsnow accepts hashing state, the current buildRowArena, and indexed candidate pairs; its documentation specifies that build columns are decoded according to the arena’s payload or Arrow-row format. - observed — Modified behavior in crates/krishiv-exec/src/join/grace.rs: Build columns for residual evaluation are now obtained from
Shared::build_columnsusing the configured row-layout mode and the indexed rows in the current arena, replacing decoding stored payload byte slices.
Reliability and maintainability
- inferred — Risk-relevant change factors for crates: blast_radius_2; blast_radius_3; direct_dependents_1; direct_dependents_2
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description clearly explains the failure, root cause, fix, and regression test. However, it omits the required Task/spec/ADR references, test commands with result lines, and the completed checklis… Add the Task/spec/ADR references, paste the exact test commands and result lines, and complete each applicable checklist item. Keep the existing failure, cause, fix, and test details. Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: decoding build rows of either layout in the reversed grace join. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description clearly explains the failure, root cause, fix, and regression test. However, it omits the required Task/spec/ADR references, test commands with result lines, and the completed checklist.
Resolution
Add the Task/spec/ADR references, paste the exact test commands and result lines, and complete each applicable checklist item. Keep the existing failure, cause, fix, and test details.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
With
krishiv.exec.join.rowLayout=true, TPC-H SF10 q21 at a 2 GiB memory limit panicked (index out of bounds decoding a zero-length row) on the grace join's reversed semi/anti path.Cause: a reversed semi or anti join with a residual condition decoded its build rows in
mark_pairsthrough the arrow-row payload accessor, which returns an empty slice for chunks stored in the row layout.Fix: candidate pairs keep the build row index, and
mark_pairsdecodes throughShared::build_columns— the path the probe side already uses — so either payload format works.Test:
a_reversal_with_a_condition_decodes_the_build_rows_of_either_layoutruns a reversed semi join with a condition in both layouts; it fails with the q21 panic on the old code and passes now. Allkrishiv-exectests pass.