fix: semantic-type and domain-validator edge cases from adversarial QA pass#136
Conversation
…al QA
- semantic_types: boolean columns (bool dtype, yes/no) were unreachable —
the distinct-support gate ran before any boolean check even though
booleans inherently have ~2 distinct values; recognise all-boolean-token
columns first (bare {0,1} stays gated as likely-numeric)
- semantic_types: a strict-format name hint (email/url/phone/postal_code)
is no longer certified when 0% of sampled values match it; the conflict
is recorded as evidence instead
- finance: FIN-006 balance check raised IndexError when every
transaction_id was missing (pandas groupby-transform with zero groups);
id-less rows are exempt, identified transactions still checked
- validate_fields: accept {'col': 'numeric'} string shorthand, raise a
clear TypeError for non-FieldSpec schema values (was AttributeError),
and warn when an unknown semantic_type has no other constraint (the
spec silently validated nothing)
All regressions covered by new tests; full suite 3238 passed / 12 skipped,
ruff + mypy clean.
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFinance validation now handles missing transaction identifiers. Field validation accepts semantic-type shorthand and validates schema entries. Semantic inference uses distinct-value detector shares, earlier boolean detection, and contradiction evidence for strict-format hints. ChangesFinance balance validation
Field schema preprocessing
Semantic type inference
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|---|---|---|---|---|---|---|---|---|---|---|
| crm | 10,200 | 40 | 1.511 | 1.516 | 13.8 | 100.0 | 0.0 | 100.0 | 93.84 | ✅ | 100.0 |
| event_log | 10,000 | 25 | 0.638 | 0.640 | 5.1 | 100.0 | 0.0 | 100.0 | 99.677 | ✅ | 100.0 |
| finance | 10,200 | 60 | 1.939 | 1.969 | 18.3 | 100.0 | 0.0 | 100.0 | 99.499 | ✅ | 100.0 |
| gold | 10,200 | 7 | 0.230 | 0.232 | 3.7 | 100.0 | 0.0 | 100.0 | 98.3 | ✅ | 100.0 |
| provenance | 10,000 | 18 | 0.498 | 0.500 | 7.2 | 100.0 | 0.0 | 100.0 | 99.765 | ✅ | 100.0 |
| wide_schema | 10,000 | 100 | 3.684 | 3.689 | 13.7 | 100.0 | 0.0 | 100.0 | 96.042 | ✅ | 100.0 |
Authored-code reduction (Metric 6)
- FreshData: 3 lines
- pandas baseline: 25 lines (88.0% reduction)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_fieldcheck.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests cover the new schema preprocessing behaviors well.
The three new tests appropriately verify string shorthand conversion,
TypeErrorfor invalid schema values, andUserWarningfor unconstrained unknown semantic types. The second half oftest_unknown_semantic_type_without_constraints_warnscorrectly useswarnings.simplefilter("error")to assert no warning is emitted when apatternconstraint is present.One minor note on line 690:
assert any(i.rule == "pattern" for i in report.issues) or report.issues— theor report.issuesfallback makes theany(...)check partially redundant, since the assertion passes if any issue exists regardless of its rule. In practice this is unlikely to mask a regression, but tightening to justassert any(i.rule == "pattern" for i in report.issues)would make the intent clearer.Also applies to: 665-690
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_fieldcheck.py` at line 6, Tighten the assertion in test_unknown_semantic_type_without_constraints_warns by removing the “or report.issues” fallback and assert directly that report.issues contains an issue whose rule is "pattern".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/freshdata/fieldcheck.py`:
- Around line 799-808: Update the warning condition in the FieldSpec validation
logic around spec.semantic_type to account for nullable and required
constraints, so it only warns when both are unset or otherwise inactive
alongside the existing constraint checks. Preserve the current warning behavior
when no executable validation constraints are configured.
- Around line 97-103: Add “postal_code” to _KNOWN_SEMANTIC_TYPES and wire it
through the existing dedicated semantic-type validation logic in the cell and
vectorized check paths. Reuse the established postal-code validator and preserve
the behavior of other recognized semantic types, eliminating the unknown-type
warning for FieldSpec(semantic_type="postal_code").
In `@src/freshdata/semantic/semantic_types.py`:
- Around line 294-308: Update the categorical branch in the semantic type
inference function to return the accumulated evidence tuple, preserving the
strict-format conflict evidence appended earlier. Keep the existing column-role
evidence and result values unchanged, and do not alter the unknown path.
---
Nitpick comments:
In `@tests/test_fieldcheck.py`:
- Line 6: Tighten the assertion in
test_unknown_semantic_type_without_constraints_warns by removing the “or
report.issues” fallback and assert directly that report.issues contains an issue
whose rule is "pattern".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bee66d8b-b996-436a-bfb3-b99b3ccb92e7
📒 Files selected for processing (6)
src/freshdata/domains/finance/validator.pysrc/freshdata/fieldcheck.pysrc/freshdata/semantic/semantic_types.pytests/domains/test_finance.pytests/test_fieldcheck.pytests/test_semantic_types.py
test_unknown_semantic_type_without_constraints_warns used
warnings.simplefilter("error") to prove the silent-fallback path stays
quiet, but that also promotes any *unrelated* warning to a hard failure
— on Python 3.9's older numpy/pandas combo, validate_fields triggers an
internal np.find_common_type DeprecationWarning unconnected to this
code, which failed CI on 3.9 only (3.10-3.13 unaffected). Now records
warnings and asserts only that no 'unknown semantic_type' warning fired.
1 participant
An adversarial QA pass over the Semantic Models (
semantic/semantic_types.py) and Domain Validators (all 8 packs) with valid/invalid/missing/malformed/duplicate/noisy/mixed-domain/multilingual/boundary/large-volume inputs. Four confirmed defects, each fixed with a regression test; no API changes.Fixes
1. Boolean columns could never be detected (
semantic/semantic_types.py)A
bool-dtype column or["yes", "no"]column has ≤2 distinct values, soMIN_DISTINCT_SUPPORT = 5returnedunknownbefore the boolean check ran —boolean_likewas unreachable for the most common boolean representations. All-boolean-token columns are now recognised ahead of the support gate; a bare{"0", "1"}column stays gated since it is just as likely numeric.2. Strict-format name hints certified contradicted types (
semantic/semantic_types.py)A column named
emailcontaining only numbers (0% of values email-shaped) was still reported asemailat 0.5 confidence. For strict-format types (email/url/phone/postal_code) a 0% content match now vetoes the name hint and records aconflictevidence entry. Genuinely inconclusive content (nonzero but below the 60% bar) still lets the name hint through unchanged.3. Finance FIN-006 crashed on all-missing transaction ids (
domains/finance/validator.py)groupby("_txn").transform("sum")raisesIndexError: cannot do a non-empty take from an empty axesin pandas when every group key is NaN. NaN-id rows were already exempt from the balance rule; they are now dropped before the groupby, so an all-NaN id column validates cleanly and identified transactions are still checked.4.
validate_fieldssilent failures at the schema boundary (fieldcheck.py){"price": "numeric"}(a natural shorthand) crashed with a rawAttributeError; string values now coerce toFieldSpec(semantic_type=...), other non-FieldSpecvalues raise a clearTypeError.semantic_typewith no other constraint silently validated nothing — the user believed the column was checked when it wasn't. This now emits aUserWarningnaming the known types. Unknown types with their own pattern/vocabulary constraints keep the documented fallback behaviour, warning-free.Verified good under adversarial input (no change needed)
"apple"in apricecolumn is preserved (never coerced/dropped), flagged byvalidate_fieldsassemantic_mismatch → quarantine, and caught by consensus inference even without a schema;"apple"as a company name and number-words in*_idcolumns are left untouched.infer_semantic_typeon 40+ edge cases: multilingual scripts, garbage bytes, unhashable payloads, embedding-vote conflicts/out-of-range scores, 200k-row columns — confidence always in [0,1], deterministic.Testing
tests/test_semantic_types.py,tests/domains/test_finance.py,tests/test_fieldcheck.py.test_name_hint_used_when_content_is_inconclusivefixture updated from 0%-matching to 3/8-matching values so it tests what its name says (inconclusive, not contradictory); the contradiction case is its own new test.Prepared with AI tooling; every finding and fix manually verified.
Summary by CodeRabbit
New Features
"numeric".Bug Fixes