fix(strings): a declared vocabulary outranks a generic null marker - #473
Merged
Merged
Conversation
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 % |
|---|
Authored-code reduction (Metric 6)
fieldcheck has applied this rule since the TestAllowedValuesBeatNullMarkers
regression -- "'NA' may be Namibia: when the schema literally allows a value,
it is a value, not a missing marker" (fieldcheck.py:466). fd.clean did not.
normalize_sentinels applied DEFAULT_SENTINELS unconditionally, so a caller who
had explicitly declared NA as permitted still lost it:
fd.validate_fields(df, {"country": FieldSpec(allowed_values=[..., "NA"])})
# -> no issue; NA is a value
fd.clean(df, context="Allowed country values are US, ..., NA.")
# policy compiles cleanly: [('country','allowed_values','soft')], 0 issues
# -> country is NaN
The same declared fact was respected by one public API and ignored by another.
'NA' is Namibia in an ISO-3166 column and 'None' is a real brand; both were
destroyed. The only escapes were protecting the column outright, which disables
every other repair, or normalize_sentinels=False, which is global and would
also stop nulling genuine "N/A" markers in every other column.
A column's declared allowed_values -- whether passed through semantic_context
or compiled from a context= policy, which lower to the same place -- now
removes those tokens from that column's sentinel set, matched casefolded and
trimmed for consistency with fieldcheck and extra_sentinels.
Deliberately narrow:
- No declaration means no change. "NA" with no vocabulary is still missing,
which is the gauntlet 'sentinel_collision' gold label, and that fixture
spells its countries out ("Namibia"), so its labels are untouched.
- A vocabulary that excludes NA still nulls it, mirroring fieldcheck's
test_na_outside_vocabulary_is_still_a_null_marker.
- The exemption is scoped to the declaring column.
- A malformed hint is ignored rather than fatal, and a bare string is rejected
so that "NA" cannot be iterated into the letters N and A.
7 of the 14 new tests fail on main; the other 7 are the must-not-change cases
and pass on both. Full suite 6588 passed / 0 failed, coverage 93.91%, and the
gauntlet gate passes with 0 corruption and 0 false positives.
kevincostner17
force-pushed
the
fix/sentinel-allowed-values
branch
from
September 16, 2026 19:19
2b12355 to
cb082ae
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fieldcheckhas applied this rule since theTestAllowedValuesBeatNullMarkersregression — "'NA' may be Namibia: when the schema literally allows a value, it is a value, not a missing marker" (fieldcheck.py:466).fd.cleandid not.normalize_sentinelsappliedDEFAULT_SENTINELSunconditionally, so a caller who had explicitly declaredNAas permitted still lost it.The same declared fact was respected by one public API and ignored by another.
NAis Namibia in an ISO-3166 column andNoneis a real brand; both were destroyed.Every route tested before this change
NAsurvivessemantic_contextsemantic_type="country"semantic_contextallowed_values=[…NA…]context="Allowed country values are …, NA."(compiles clean, 0 issues)preserve_columnscontext="Never modify country values."→protectednormalize_sentinels=FalseSo the only escapes were protecting the column outright or turning sentinel handling off for the whole frame — which would also stop nulling genuine
"N/A"markers elsewhere.The fix
A column's declared
allowed_values— viasemantic_contextor compiled from acontext=policy, which lower to the same place — now removes those tokens from that column's sentinel set, matched casefolded and trimmed for consistency withfieldcheckandextra_sentinels.Deliberately narrow:
"NA"with no vocabulary is still missing — the gauntletsentinel_collisiongold label. That fixture spells its countries out ("Namibia"), so its labels are untouched and the gate still passes.NAstill nulls it, mirroringfieldcheck'stest_na_outside_vocabulary_is_still_a_null_marker.NAforcountrydoes not rescue it innote."NA"cannot be iterated into the lettersNandA.The gauntlet fixture states the intended behaviour outright: "without a vocabulary containing 'NA', the null-marker reading wins; with allowed_values that includes 'NA' the value survives".
Verification
tests/test_sentinel_allowed_values.py: 14 cases. 7 fail onmain; the other 7 are the must-not-change cases and pass on both, showing the default is untouched.python -m benchmarks.gauntlet run --check: all gates passed, 0 corruption and 0 false-positive rate across all five fixtures.ruff check/ruff formatclean. (Localmypyis blocked by the numpy 2.5 stubs issue; CI pinsnumpy<2.5.)