[iceboundrock]

Why

search_pattern executed the table's plan twice. The first run read the columns its steps use and found the positions of the matches. The second re-sorted the full table, concatenated every column into one batch, and took the rows at those positions. That had three costs:

• The source was scanned twice, and an expensive upstream chain (join, derive) ran twice.
• concat_batches deep-copied the whole table, so peak memory was about twice the table, even though matches are usually a tiny fraction of rows.
• It was not correct with tied sort keys. Two independent sorts need not put tied rows in the same order, and batch boundaries differ between runs. A position from the first run could therefore name a row the steps never matched. The issue comments make this point, and it rules out reusing (batch_idx, row_idx) from a projected first run.

What changed

search_pattern_impl (src/ops/pattern_match.rs) now:

1. Executes table.ordered_df() once with every column. This is the table-order path AGENTS.md requires, so the old hand-built second sort is gone.
2. Evaluates each step predicate per batch and joins only the boolean masks and the partition_by column across batches.
3. Scans once, as before. The single-step special case now goes through the same loop.
4. Takes the matching rows from the batches of that same execution with arrow::compute::interleave, column by column, so the full rows are never concatenated.

Peak memory is the collected table, plus the masks and the partition column, plus the matching rows.

Not changed:

• search_pattern_count. The issue comments say its separate fast path does not need to follow this refactor, so its pruned projected path is untouched. Because of that, the two functions no longer share steps 1 to 8, so I did not extract a common pipeline.
• The validation error for a pattern that names no column of the table.
• The empty-result returns.

The CHANGELOG has a Fixed entry describing both the double execution and the tied-key row mix-up.

How it was tested

• New Rust test search_pattern_executes_the_plan_once. Its StreamingTable source counts execute calls, and the test asserts the source is executed exactly once. It also checks the full result rows. It covers a shuffled source sorted by k, with and without partition_by, and an in-order source split so that matches cross batch boundaries. The session batch size is 3, so the sorted output arrives in several batches. Against the old code the count would be 2, since it called collect() on two plans over the same source.
• New py-ltseq/tests/test_search_pattern_rows.py:
  • Every column, including ones no step reads (id, note), and the result order are compared with a Python reference at every data_layout.
  • At every engine_layout, with 10-way ties in the sort key and 7-row input batches, the returned rows are exactly the rows the steps matched, in key order, without duplicates.
• pytest over all test_search_pattern*.py files: 451 passed.
• pytest over every other test file that calls search_pattern, plus test_no_materialization_rule.py: 1594 passed.
• cargo test: 191 passed.
• cargo clippy --all-targets -- -D warnings: clean.

The full Python suite was not run locally; CI runs it on this PR.

Closes #149

[iceboundrock]

AI Code Review — Round 1

Reviewed HEAD: 2445e2d22394296cb6ff7efb9e78e31b2846ef59 against base main (merge base 86cf9efbc5712f2381089b441e00a66efff3c33e)

Findings

None.

Spec

The implementation addresses issue #149: matching predicates and returned rows use the same full-column execution, eliminating positional correspondence between independent sorts. The execution-count test verifies one source execution. Regression tests cover multiple batches, tied sort keys, partition boundaries, all output columns and result order. Full-table concatenation is removed from the row-returning path.

Standards

Consistent with AGENTS.md and CLAUDE.md: execution uses ordered_df(), remains in Rust under the existing detached execution boundary, and adds regression coverage and an Unreleased changelog entry. No public signature changes require stub updates. The worktree remains unchanged.

Assessment

No actionable correctness, security or maintainability defects found in the bound diff and related code. Batch-local positions are derived from the same collected batches used for predicate evaluation, and the unified matching loop preserves single-step and multi-step behavior.

Observations

The supplied PR metadata matches the bound HEAD and base branch. All supplied CI checks were pending. The memory improvement is supported structurally by removing the full-row concatenation; peak RSS was not measured during this review. Memory still includes predicate masks, partition bookkeeping and matching-position vectors in addition to input and output rows.

Verification

Inspected the complete bound diff, related implementation, repository guidance, issue discussion, PR description and supplied checks. All 451 tests across test_search_pattern_count.py, test_search_pattern_rows.py, test_search_pattern_coercion.py and test_search_pattern_null_logic.py passed. cargo test --offline passed all 191 Rust tests, including the execution-count regression. cargo clippy --offline --all-targets -- -D warnings passed. git diff --check passed. An initial Python run exceeded its two-minute limit; its complete rerun passed. A broader Python regression run exceeded ten minutes without completing and is not counted as passing. No test processes remain running.

Summary

No findings requiring another fix round. The single-execution design resolves the duplicate work and tied-key row mismatch, with passing targeted Python and Rust coverage. Pending CI and unmeasured peak memory remain verification limitations.

Needs another fix round: NO