Skip to content

feat: assert no unnamed nodes and no null or empty values in final graph QC - #106

Merged
SkyeAv merged 1 commit into
mainfrom
qc-no-empty-null-values
Aug 21, 2026
Merged

feat: assert no unnamed nodes and no null or empty values in final graph QC#106
SkyeAv merged 1 commit into
mainfrom
qc-no-empty-null-values

Conversation

@SkyeAv

@SkyeAv SkyeAv commented Aug 21, 2026

Copy link
Copy Markdown
Owner

The --qc stage-7 study pass now asserts that every node in the final NDJSON carries a name and that no field in either output file is null or empty — checked recursively — failing the build on the first violation instead of letting it ship.

Study assertions (src/tablassert/study.py)

  • unnamed-nodes: a node record whose name key is missing, null, or strips to empty fails the study; offenders are keyed by node id (or <no id>), capped at 10 examples like the other assertions. On pipeline output the missing-key branch is what fires — the writer's strip_nulls deletes empty and null-like names before the file is written, and was verified to apply to the nodes file too (dedup_stream(nodes_tmp, is_edges=False)finalize_recordstrip_nulls in rust/src/ndjson.rs).
  • empty-or-null-values: any field in the nodes or edges file whose value is JSON null, a string that strips to empty, or an empty container fails, counted per field with examples like name (2); recursion means a null nested inside an attributes list counts under its top-level field.

Design

  • Stricter than the writer, deliberately. strip_nulls (rust/src/json.rs) scrubs dict entries at every depth but passes array scalars (["x", ""], ["x", null]) and emptied nested objects ({}, [{}]) through verbatim; the study asserts the stronger no-empty-anywhere contract so the first such shape to reach an emitted file fails loudly rather than shipping silently. Deferred: tightening the Rust strip_nulls to scrub array scalars and drop emptied nested dicts, so writer and study agree at the source.
  • Null-like strings not flagged. NA/NaN/null/none are dropped by the writer's bad-token sweep but are neither null nor empty; the assertion targets absent values, not spellings.
  • original_* exemption narrowed to whitespace. Verbatim source padding stays exempt from whitespace-values (the PR fix(qc): allow whitespace in original_* fields in the KGX study #90 convention), but empty/null original_* values are now flagged — the writer drops them everywhere, so they are never legitimate. Accepted caveat: a whitespace-only value intentionally trips both whitespace-values and empty-or-null-values.
  • Falsy-but-meaningful values pass. 0 and false are kept by the writer and pass the study (p_value: 0, negated: false), pinned by test.

Docs

  • docs/cli.md (--qc row), the build_kg docstring in src/tablassert/cli.py (rendered by --help), and CHANGELOG.md (Unreleased).

Testing

  • uv run --no-sync pytest -q --no-cov1050 passed, 15 skipped
  • uv run --no-sync pytest tests/test_study.py --no-cov -q21 passed, study.py at 100% line coverage
  • uv run --no-sync ruff check + ruff format --check → clean; uv run --no-sync pyright src/tablassert/study.py src/tablassert/cli.py tests/test_study.py0 errors

Summary by CodeRabbit

  • Quality Checks

    • Strengthened build-kg --qc auditing to detect unnamed nodes, duplicate or undeclared nodes, malformed lines, whitespace issues, and recursively empty or null values.
    • Added capped violation examples for easier review.
    • Preserved valid falsy values and null-like strings such as "NA" and "null".
  • Documentation

    • Updated CLI guidance and the unreleased changelog to describe the expanded graph quality checks.
  • Tests

    • Expanded coverage for nested empty values, missing names or IDs, original fields, and valid exceptions.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: db02faa1-9253-4242-a216-dfc1313ae47b

📥 Commits

Reviewing files that changed from the base of the PR and between 8d497d9 and f2dacfe.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • docs/cli.md
  • src/tablassert/cli.py
  • src/tablassert/study.py
  • tests/test_study.py

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


📝 Walkthrough

Walkthrough

Stage-7 graph QC now recursively rejects null or empty values and rejects nodes without non-empty names. The scan reports capped violation examples, while original_* fields remain exempt only from whitespace checks. Tests and documentation cover the updated behavior.

Changes

Stage-7 Graph QC

Layer / File(s) Summary
Recursive value and node validation
src/tablassert/study.py
The NDJSON scan recursively detects null, blank, and empty-container values. It also records nodes with missing, null, or blank names. Null-like strings remain exempt.
QC reporting and coverage
src/tablassert/study.py, tests/test_study.py, docs/cli.md, src/tablassert/cli.py, CHANGELOG.md
study_kgx emits capped violations for the new checks. Tests cover nested values, name handling, exemptions, and fixture updates. CLI documentation and the changelog describe the expanded QC checks.

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

Merge Risk: ⚪ Minimal · up to f2dac

This change strengthens final graph validation for unnamed, null, and empty values while documenting and testing the behavior; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant study_kgx
  participant _scan_ndjson
  participant _is_empty_or_null
  study_kgx->>_scan_ndjson: scan emitted NDJSON
  _scan_ndjson->>_is_empty_or_null: inspect field values recursively
  _is_empty_or_null-->>_scan_ndjson: return empty or null result
  _scan_ndjson-->>study_kgx: return QC counters
  study_kgx-->>study_kgx: emit capped violations
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main QC changes for unnamed nodes and null or empty values.
Description check ✅ Passed The description thoroughly covers the change, design decisions, documentation updates, and exact test results, but omits the template checklist and related-issues section.
Docstring Coverage ✅ Passed Docstring coverage is 94.74% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (2 skipped: 2 unsupported.)
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch qc-no-empty-null-values

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.

@SkyeAv
SkyeAv merged commit 94ad57e into main Aug 21, 2026
5 checks passed
@SkyeAv
SkyeAv deleted the qc-no-empty-null-values branch August 21, 2026 17:32
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