Skip to content

perf: scope transformation label reads to datasets - #23

Merged
TonisOrmisson merged 2 commits into
mainfrom
perf/dataset-label-reads
Sep 9, 2026
Merged

TonisOrmisson merged 2 commits into
mainfrom
perf/dataset-label-reads

Conversation

@TonisOrmisson

@TonisOrmisson TonisOrmisson commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Phase 6b: scope transformation input-schema value-label reads to the target dataset's variables.

  • Add one ownership join/predicate to the existing label SELECT in _input_schema.
  • Preserve target variables' labels, including shared sets whose value_label_set.dataset_id belongs to another dataset; preserve label order, typed values, diagnostics, transactions and public APIs.
  • Unrelated label rows are no longer converted. This removes irrelevant malformed-label failures without adding a new ownership-validation rule.
  • Export wide.py, caches, schema/index changes, dependencies and other label reads remain out of scope.

RED/GREEN and checks

  • RED 1f4b89b: six intentional failures on base—unrelated label rows were fetched at 0/3/40 growth and unrelated malformed labels raised before binding; target-linked malformed behavior already passed.
  • GREEN c195a25: production only; tests immutable after RED.
  • Focused local suite: 92 passed, 9 skipped.
  • Full non-service suite: 402 passed, 9 skipped, 53 deselected.
  • Independent read-only full-diff review: No findings.
  • All 29 GitHub checks are successful and merge state is CLEAN. CodeRabbit completed with no actionable comments and minimal merge risk; no review threads. The generic docstring-percentage warning is non-actionable repository boilerplate, not a missing contract.
  • @codex review was requested at 5597433429, but the Codex service returned an account usage-limit message (5597434497) rather than running a review. This is recorded as an external review blocker; no Codex result is claimed.

Evidence

Immutable base c6cf9e593e9c30764dac585449db19a98de60b62 versus head c195a25dd43b70c4b71fecfb69c2fbbf58578fed, same Python 3.13.2 / SQLAlchemy 2.0.52 / SQLite 3.47.1, disposable SQLite only.

Surface Base label rows After label rows Base → after fetched rows at 10k unrelated labels
Loader 7 5 10,011 → 9
Canonical apply 7 5 10,912 → 910
SPSS apply 14 10 20,923 → 919

SQL statement counts remain unchanged: loader 3, canonical 213, SPSS 216. With fixed target labels, after-change label rows remain 5/5/10 for 0, 10,000 unrelated variables and 10,000 unrelated labels. Target data, metadata, table/dataset identity, audit contents and validation results matched base; only generated audit IDs/timestamps were normalized for comparison. 166 compatibility assertions passed across 24 scenarios per revision.

Malformed behavior is explicit: an unrelated-only numeric NULL no longer poisons the target load/apply; a target-linked NULL—including a foreign-owned/shared set—still raises the exact existing TypeError before mutation/audit. Full raw evidence and commands: ../python-label-reads-evidence.md and /tmp/python-label-reads-phase6b-VSZrJV, outside the repository. No timing or server-performance claim is made.

Next phase

After user merge, proceed to Phase 7: measure memory copies and bounded batches; full streaming only if justified separately. Overall phases 0–9 remain unfinished. Do not auto-merge.

@TonisOrmisson

Copy link
Copy Markdown
Contributor Author

@codex review

@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: 94eb55b4-7941-4c1e-b81d-1d1af37278e6

📥 Commits

Reviewing files that changed from the base of the PR and between c6cf9e5 and c195a25.

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

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


📝 Walkthrough

Walkthrough

The value-label query now filters through each linked variable’s dataset. Tests cover dataset-scoped reads across loader, canonical, and SPSS surfaces, including shared labels, read counts, and malformed label isolation.

Changes

Value-label dataset scoping

Layer / File(s) Summary
Dataset-scoped value-label query
src/openstatspec/sql/inplace_transform.py
The value-label query joins variable through variable_value_label_set.variable_id and filters by variable.dataset_id.
Label isolation and malformed-data tests
tests/test_inplace_transform.py
Tests create shared label sets, count SQLite reads across supported surfaces, and verify behavior for unrelated and malformed labels.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c195a

Transformation schema loading now ignores value labels belonging only to sibling datasets, preventing unrelated malformed labels and excess reads from affecting target transformations. Target behavior is covered across loader, canonical, and SPSS paths, with no remaining merge-blocking risk identified.

🚥 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 15 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: limiting transformation label reads to the relevant datasets.
  • 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/dataset-label-reads

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.

@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.

@TonisOrmisson
TonisOrmisson merged commit 06b346a into main Sep 9, 2026
29 checks passed
@TonisOrmisson
TonisOrmisson deleted the perf/dataset-label-reads branch September 9, 2026 07:03
@TonisOrmisson

Copy link
Copy Markdown
Contributor Author

@codex review

@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.

@TonisOrmisson

Copy link
Copy Markdown
Contributor Author

@codex review

@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.

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