Skip to content

Commit 6f53878

Browse files
fix(strings): a declared vocabulary outranks a generic null marker (#473)
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. Co-authored-by: Kevin Costner <kiran.gangalakunta@gmail.com>
1 parent 2dbd4bd commit 6f53878

3 files changed

Lines changed: 199 additions & 11 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,21 @@ adheres to [Semantic Versioning](https://semver.org/).
2424
is now rejected instead of being coerced to `123` and `120.5`. US and Indian
2525
lakh formats are unchanged. **Default-output change:** a European-format
2626
currency string in a money column now cleans to its correct magnitude.
27+
- A value the caller has explicitly declared permitted is no longer discarded
28+
as a null marker. `fd.clean`'s `normalize_sentinels` step applied the
29+
built-in sentinel set unconditionally, so `"NA"` in an ISO-3166 country
30+
column (Namibia) and `"None"` in a brand column became missing even when
31+
`allowed_values` for that column listed them. `fd.validate_fields` has
32+
honoured the opposite rule since the `TestAllowedValuesBeatNullMarkers`
33+
regression — "when the schema literally allows a value, it is a value, not a
34+
missing marker" — so the same declaration was respected by one public API
35+
and ignored by another. The only previous escapes were protecting the column
36+
outright, which disables every other repair, or `normalize_sentinels=False`,
37+
which is global. A column's declared `allowed_values` (whether passed through
38+
`semantic_context` or compiled from a `context=` policy) now removes those
39+
tokens from that column's sentinel set, matched casefolded and trimmed. The
40+
exemption is scoped to the declaring column, and a column with no declaration
41+
is unchanged — `"NA"` with no vocabulary is still read as missing.
2742
- `engine="duckdb"` no longer silently changes temporal values on the fully
2843
native path (`strategy="conservative"`, `fix_dtypes=False`). A nanosecond
2944
`timedelta64[ns]` column was truncated to DuckDB's microsecond `INTERVAL`

‎src/freshdata/steps/strings.py‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -162,16 +162,40 @@ def _text_columns(df: pd.DataFrame) -> list:
162162
]
163163

164164

165+
def declared_vocabulary(config: CleanConfig, column: object) -> frozenset[str]:
166+
"""Casefolded ``allowed_values`` the caller declared for *column*.
167+
168+
An explicit vocabulary outranks a generic null marker: ``"NA"`` is Namibia
169+
in an ISO-3166 column and ``"None"`` is a real brand. ``fieldcheck`` has
170+
applied that rule since the TestAllowedValuesBeatNullMarkers regression;
171+
this is the same rule for the cleaning path.
172+
"""
173+
context = getattr(config, "semantic_context", None)
174+
if not isinstance(context, dict):
175+
return frozenset()
176+
columns = context.get("columns")
177+
if not isinstance(columns, dict):
178+
return frozenset()
179+
hints = columns.get(str(column))
180+
if not isinstance(hints, dict):
181+
return frozenset()
182+
allowed = hints.get("allowed_values")
183+
if not allowed or isinstance(allowed, (str, bytes)):
184+
return frozenset()
185+
try:
186+
return frozenset(str(v).casefold().strip() for v in allowed)
187+
except TypeError:
188+
return frozenset()
189+
190+
165191
def clean_strings(df: pd.DataFrame, config: CleanConfig, report: CleanReport) -> pd.DataFrame:
166192
"""Apply whitespace stripping and sentinel→missing to text-capable columns.
167193
168194
Categorical columns with text categories are repaired too; they keep their
169195
categorical dtype (see :func:`normalize_categorical`).
170196
"""
171197
if not (
172-
config.strip_whitespace
173-
or config.normalize_sentinels
174-
or config.string_case is not None
198+
config.strip_whitespace or config.normalize_sentinels or config.string_case is not None
175199
):
176200
return df
177201
sentinels = active_sentinels(config)
@@ -182,20 +206,34 @@ def clean_strings(df: pd.DataFrame, config: CleanConfig, report: CleanReport) ->
182206
if str(col) in protected:
183207
continue # context-protected columns must stay byte-identical
184208
s = df[col]
209+
# A token the caller declared as a permitted value is a value, not a
210+
# missing marker, so it is removed from this column's sentinel set.
211+
column_sentinels = sentinels - declared_vocabulary(config, col)
185212
normalize = (
186213
normalize_categorical if isinstance(s.dtype, pd.CategoricalDtype) else normalize_text
187214
)
188-
normalized, n_stripped, n_sentinels, n_case = normalize(s, config, sentinels)
215+
normalized, n_stripped, n_sentinels, n_case = normalize(s, config, column_sentinels)
189216
if n_stripped:
190-
report.add("strip_whitespace", "trimmed surrounding whitespace",
191-
column=str(col), count=n_stripped)
217+
report.add(
218+
"strip_whitespace",
219+
"trimmed surrounding whitespace",
220+
column=str(col),
221+
count=n_stripped,
222+
)
192223
if n_sentinels:
193-
report.add("normalize_sentinels",
194-
'replaced sentinel strings ("N/A", "-", "", …) with missing',
195-
column=str(col), count=n_sentinels)
224+
report.add(
225+
"normalize_sentinels",
226+
'replaced sentinel strings ("N/A", "-", "", …) with missing',
227+
column=str(col),
228+
count=n_sentinels,
229+
)
196230
if n_case:
197-
report.add("normalize_case", f"converted text to {config.string_case}",
198-
column=str(col), count=n_case)
231+
report.add(
232+
"normalize_case",
233+
f"converted text to {config.string_case}",
234+
column=str(col),
235+
count=n_case,
236+
)
199237
if n_stripped or n_sentinels or n_case:
200238
df[col] = normalized
201239
return df
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
"""A declared vocabulary outranks a generic null marker in fd.clean (FD2-001).
2+
3+
``fieldcheck`` has applied this rule since the ``TestAllowedValuesBeatNullMarkers``
4+
regression: "'NA' may be Namibia: when the schema literally allows a value, it
5+
is a value, not a missing marker" (``fieldcheck.py:466``). ``fd.clean`` did not.
6+
``normalize_sentinels`` applied ``DEFAULT_SENTINELS`` unconditionally, so a
7+
caller who had explicitly declared ``NA`` as permitted still lost it, and the
8+
only escapes were protecting the column outright (which disables every other
9+
repair) or turning sentinel handling off globally for every column.
10+
11+
The gauntlet fixture states the intended behaviour directly: "without a
12+
vocabulary containing 'NA', the null-marker reading wins; with allowed_values
13+
that includes 'NA' the value survives".
14+
"""
15+
16+
from __future__ import annotations
17+
18+
import pandas as pd
19+
import pytest
20+
21+
import freshdata as fd
22+
23+
ISO = ["US", "GB", "FR", "DE", "JP", "CA", "AU", "BR", "NA"]
24+
25+
26+
def _frame():
27+
return pd.DataFrame({"cust": [f"c{i}" for i in range(9)], "country": list(ISO)})
28+
29+
30+
def test_declared_allowed_values_keep_a_sentinel_looking_value():
31+
"""'NA' is Namibia when the caller says the column allows it."""
32+
out = fd.clean(
33+
_frame(),
34+
verbose=False,
35+
semantic_context={"columns": {"country": {"allowed_values": ISO}}},
36+
)
37+
assert out["country"].iloc[8] == "NA"
38+
39+
40+
def test_a_compiled_context_policy_reaches_the_sentinel_step():
41+
"""The documented natural-language route must work as well as the dict."""
42+
out = fd.clean(
43+
_frame(),
44+
verbose=False,
45+
context="Allowed country values are US, GB, FR, DE, JP, CA, AU, BR, NA.",
46+
)
47+
assert out["country"].iloc[8] == "NA"
48+
49+
50+
def test_without_a_declaration_the_null_marker_reading_still_wins():
51+
"""Unchanged default. This is the gauntlet 'sentinel_collision' gold label."""
52+
out = fd.clean(_frame(), verbose=False)
53+
assert pd.isna(out["country"].iloc[8])
54+
55+
56+
def test_a_vocabulary_that_excludes_na_still_nulls_it():
57+
"""Mirrors fieldcheck's test_na_outside_vocabulary_is_still_a_null_marker."""
58+
out = fd.clean(
59+
_frame(),
60+
verbose=False,
61+
semantic_context={"columns": {"country": {"allowed_values": ["US", "GB", "FR"]}}},
62+
)
63+
assert pd.isna(out["country"].iloc[8])
64+
65+
66+
def test_the_exemption_is_scoped_to_the_declaring_column():
67+
"""Declaring NA for one column must not rescue it everywhere."""
68+
df = pd.DataFrame(
69+
{
70+
"country": ["US", "NA", "DE", "FR", "GB", "JP", "CA", "AU", "BR"],
71+
"note": ["a", "NA", "c", "d", "e", "f", "g", "h", "i"],
72+
}
73+
)
74+
out = fd.clean(
75+
df,
76+
verbose=False,
77+
semantic_context={"columns": {"country": {"allowed_values": ISO}}},
78+
)
79+
assert out["country"].iloc[1] == "NA"
80+
assert pd.isna(out["note"].iloc[1])
81+
82+
83+
@pytest.mark.parametrize("declared", ["na", "Na", " NA "])
84+
def test_vocabulary_matching_is_casefolded_and_trimmed(declared):
85+
"""Consistent with fieldcheck and with extra_sentinels normalisation."""
86+
out = fd.clean(
87+
_frame(),
88+
verbose=False,
89+
semantic_context={"columns": {"country": {"allowed_values": ["US", declared]}}},
90+
)
91+
assert out["country"].iloc[8] == "NA"
92+
93+
94+
@pytest.mark.parametrize("allowed", [None, [], "NA", 42, {"nested": "dict"}])
95+
def test_a_malformed_vocabulary_is_ignored_not_fatal(allowed):
96+
"""A bad hint must not crash cleaning, and must not grant an exemption.
97+
98+
A bare string is rejected deliberately: iterating it would treat 'N' and
99+
'A' as separate permitted values.
100+
"""
101+
out = fd.clean(
102+
_frame(),
103+
verbose=False,
104+
semantic_context={"columns": {"country": {"allowed_values": allowed}}},
105+
)
106+
assert pd.isna(out["country"].iloc[8])
107+
108+
109+
def test_the_none_brand_case_behaves_the_same_way():
110+
"""'None' is a real brand; the same declaration rescues it."""
111+
df = pd.DataFrame(
112+
{
113+
"k": [f"r{i}" for i in range(9)],
114+
"brand": [
115+
"Acme",
116+
"Globex",
117+
"Initech",
118+
"Stark",
119+
"Wayne",
120+
"Hooli",
121+
"Umbrella",
122+
"Vandelay",
123+
"None",
124+
],
125+
}
126+
)
127+
plain = fd.clean(df, verbose=False)
128+
assert pd.isna(plain["brand"].iloc[8])
129+
130+
declared = fd.clean(
131+
df,
132+
verbose=False,
133+
semantic_context={"columns": {"brand": {"allowed_values": [*df["brand"].tolist()]}}},
134+
)
135+
assert declared["brand"].iloc[8] == "None"

0 commit comments

Comments
 (0)