Skip to content

perf: reuse canonical import rows - #27

Merged
TonisOrmisson merged 1 commit into
mainfrom
perf/reuse-import-rows
Sep 9, 2026
Merged

TonisOrmisson merged 1 commit into
mainfrom
perf/reuse-import-rows

Conversation

@TonisOrmisson

@TonisOrmisson TonisOrmisson commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • reuse the already-private canonical row list after preflight instead of building a second full list/dict materialization;
  • preserve caller-owned mappings and existing supplied __case_ordinal precedence with setdefault;
  • cover one-shot inputs, Decimal/binary64/NULL/string fidelity, bounded batches, supplied ordinals, empty input, and catalog case counts.

Scope

One production file and one test file:

  • src/openstatspec/sql/wide.py
  • tests/test_atomic_import.py

No API, dependency, configuration, SQL/profile, schema, batch-size, streaming, or cleanup redesign.

Evidence

Disposable baseline/candidate subprocesses used fresh SQLite files and identical dependencies with 50,000 rows, narrow 5-variable and wide 64-variable mixed shapes; one warm-up and five alternating fresh pairs per shape. Input construction was outside the timed call and readback/count checks were outside timing.

  • tracemalloc peak median: narrow 51,707,649 → 42,089,738 bytes (-18.6%); wide 306,092,516 → 227,165,756 bytes (-25.8%);
  • peak RSS median: narrow 229,612 → 212,048 KiB (-7.6%); wide 802,840 → 717,192 KiB (-10.7%);
  • import wall-time medians without tracemalloc: narrow 0.778 → 0.772 s; wide 4.011 → 3.850 s; observed ranges overlap and no material regression was observed.

Baseline: Python PR #26 merge 127017a1771183553beabdb8fff2a2186a432576; candidate: bfd281b.

Verification

  • RED confirmed baseline row/list identity assertion failed; behavior tests passed.
  • focused import/catalog/profile suite: 118 passed, 8 skipped;
  • non-service suite: 411 passed, 9 skipped, 53 deselected;
  • git diff --check;
  • local read-only review: no findings.

Please do not auto-merge; maintainer review/merge remains required.

Summary by CodeRabbit

  • Bug Fixes
    • Improved dataset imports to preserve numeric precision, Unicode, whitespace, and caller-supplied case ordering.
    • Empty imports now correctly produce no rows and report a zero case count.
    • Import processing now reuses canonicalized data consistently, improving reliability during batching.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b8e3788f-1064-4540-9d58-e53c0ee17d3d

📥 Commits

Reviewing files that changed from the base of the PR and between 127017a and bfd281b.

📒 Files selected for processing (2)
  • src/openstatspec/sql/wide.py
  • tests/test_atomic_import.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The wide dataset import now reuses source_rows after canonicalization instead of creating a separate materialized collection. Tests cover row identity, input immutability, value preservation, supplied ordinals, and empty input.

Changes

Wide import row reuse

Layer / File(s) Summary
Reuse source rows during insertion
src/openstatspec/sql/wide.py
The import assigns missing case ordinals to source_rows and uses them for case counts, batching, insertion, and final reporting.
Validate import behavior
tests/test_atomic_import.py
Tests verify canonical row reuse, unchanged inputs, preserved values, supplied ordinals, and zero rows for empty input.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to bfd28

Wide imports now reuse canonicalized rows to reduce memory use while preserving row values, ordinal behavior, counts, and caller-owned input mappings. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: reusing canonical import rows to improve performance.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/reuse-import-rows

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@TonisOrmisson
TonisOrmisson merged commit 40466ea into main Sep 9, 2026
29 checks passed
@TonisOrmisson
TonisOrmisson deleted the perf/reuse-import-rows branch September 9, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant