From cb082ae964b3b9c1778209e9b452520b216237a6 Mon Sep 17 00:00:00 2001 From: Kevin Costner Date: Thu, 17 Sep 2026 00:39:13 +0530 Subject: [PATCH] fix(strings): a declared vocabulary outranks a generic null marker 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. --- CHANGELOG.md | 15 +++ src/freshdata/steps/strings.py | 60 +++++++++--- tests/test_sentinel_allowed_values.py | 135 ++++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 tests/test_sentinel_allowed_values.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6315376..f410e7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,21 @@ adheres to [Semantic Versioning](https://semver.org/). is now rejected instead of being coerced to `123` and `120.5`. US and Indian lakh formats are unchanged. **Default-output change:** a European-format currency string in a money column now cleans to its correct magnitude. +- A value the caller has explicitly declared permitted is no longer discarded + as a null marker. `fd.clean`'s `normalize_sentinels` step applied the + built-in sentinel set unconditionally, so `"NA"` in an ISO-3166 country + column (Namibia) and `"None"` in a brand column became missing even when + `allowed_values` for that column listed them. `fd.validate_fields` has + honoured the opposite rule since the `TestAllowedValuesBeatNullMarkers` + regression — "when the schema literally allows a value, it is a value, not a + missing marker" — so the same declaration was respected by one public API + and ignored by another. The only previous escapes were protecting the column + outright, which disables every other repair, or `normalize_sentinels=False`, + which is global. A column's declared `allowed_values` (whether passed through + `semantic_context` or compiled from a `context=` policy) now removes those + tokens from that column's sentinel set, matched casefolded and trimmed. The + exemption is scoped to the declaring column, and a column with no declaration + is unchanged — `"NA"` with no vocabulary is still read as missing. - `engine="duckdb"` no longer silently changes temporal values on the fully native path (`strategy="conservative"`, `fix_dtypes=False`). A nanosecond `timedelta64[ns]` column was truncated to DuckDB's microsecond `INTERVAL` diff --git a/src/freshdata/steps/strings.py b/src/freshdata/steps/strings.py index d89c250..1571be0 100644 --- a/src/freshdata/steps/strings.py +++ b/src/freshdata/steps/strings.py @@ -162,6 +162,32 @@ def _text_columns(df: pd.DataFrame) -> list: ] +def declared_vocabulary(config: CleanConfig, column: object) -> frozenset[str]: + """Casefolded ``allowed_values`` the caller declared for *column*. + + An explicit vocabulary outranks a generic null marker: ``"NA"`` is Namibia + in an ISO-3166 column and ``"None"`` is a real brand. ``fieldcheck`` has + applied that rule since the TestAllowedValuesBeatNullMarkers regression; + this is the same rule for the cleaning path. + """ + context = getattr(config, "semantic_context", None) + if not isinstance(context, dict): + return frozenset() + columns = context.get("columns") + if not isinstance(columns, dict): + return frozenset() + hints = columns.get(str(column)) + if not isinstance(hints, dict): + return frozenset() + allowed = hints.get("allowed_values") + if not allowed or isinstance(allowed, (str, bytes)): + return frozenset() + try: + return frozenset(str(v).casefold().strip() for v in allowed) + except TypeError: + return frozenset() + + def clean_strings(df: pd.DataFrame, config: CleanConfig, report: CleanReport) -> pd.DataFrame: """Apply whitespace stripping and sentinel→missing to text-capable columns. @@ -169,9 +195,7 @@ def clean_strings(df: pd.DataFrame, config: CleanConfig, report: CleanReport) -> categorical dtype (see :func:`normalize_categorical`). """ if not ( - config.strip_whitespace - or config.normalize_sentinels - or config.string_case is not None + config.strip_whitespace or config.normalize_sentinels or config.string_case is not None ): return df sentinels = active_sentinels(config) @@ -182,20 +206,34 @@ def clean_strings(df: pd.DataFrame, config: CleanConfig, report: CleanReport) -> if str(col) in protected: continue # context-protected columns must stay byte-identical s = df[col] + # A token the caller declared as a permitted value is a value, not a + # missing marker, so it is removed from this column's sentinel set. + column_sentinels = sentinels - declared_vocabulary(config, col) normalize = ( normalize_categorical if isinstance(s.dtype, pd.CategoricalDtype) else normalize_text ) - normalized, n_stripped, n_sentinels, n_case = normalize(s, config, sentinels) + normalized, n_stripped, n_sentinels, n_case = normalize(s, config, column_sentinels) if n_stripped: - report.add("strip_whitespace", "trimmed surrounding whitespace", - column=str(col), count=n_stripped) + report.add( + "strip_whitespace", + "trimmed surrounding whitespace", + column=str(col), + count=n_stripped, + ) if n_sentinels: - report.add("normalize_sentinels", - 'replaced sentinel strings ("N/A", "-", "", …) with missing', - column=str(col), count=n_sentinels) + report.add( + "normalize_sentinels", + 'replaced sentinel strings ("N/A", "-", "", …) with missing', + column=str(col), + count=n_sentinels, + ) if n_case: - report.add("normalize_case", f"converted text to {config.string_case}", - column=str(col), count=n_case) + report.add( + "normalize_case", + f"converted text to {config.string_case}", + column=str(col), + count=n_case, + ) if n_stripped or n_sentinels or n_case: df[col] = normalized return df diff --git a/tests/test_sentinel_allowed_values.py b/tests/test_sentinel_allowed_values.py new file mode 100644 index 0000000..8a61c60 --- /dev/null +++ b/tests/test_sentinel_allowed_values.py @@ -0,0 +1,135 @@ +"""A declared vocabulary outranks a generic null marker in fd.clean (FD2-001). + +``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, and the +only escapes were protecting the column outright (which disables every other +repair) or turning sentinel handling off globally for every column. + +The gauntlet fixture states the intended behaviour directly: "without a +vocabulary containing 'NA', the null-marker reading wins; with allowed_values +that includes 'NA' the value survives". +""" + +from __future__ import annotations + +import pandas as pd +import pytest + +import freshdata as fd + +ISO = ["US", "GB", "FR", "DE", "JP", "CA", "AU", "BR", "NA"] + + +def _frame(): + return pd.DataFrame({"cust": [f"c{i}" for i in range(9)], "country": list(ISO)}) + + +def test_declared_allowed_values_keep_a_sentinel_looking_value(): + """'NA' is Namibia when the caller says the column allows it.""" + out = fd.clean( + _frame(), + verbose=False, + semantic_context={"columns": {"country": {"allowed_values": ISO}}}, + ) + assert out["country"].iloc[8] == "NA" + + +def test_a_compiled_context_policy_reaches_the_sentinel_step(): + """The documented natural-language route must work as well as the dict.""" + out = fd.clean( + _frame(), + verbose=False, + context="Allowed country values are US, GB, FR, DE, JP, CA, AU, BR, NA.", + ) + assert out["country"].iloc[8] == "NA" + + +def test_without_a_declaration_the_null_marker_reading_still_wins(): + """Unchanged default. This is the gauntlet 'sentinel_collision' gold label.""" + out = fd.clean(_frame(), verbose=False) + assert pd.isna(out["country"].iloc[8]) + + +def test_a_vocabulary_that_excludes_na_still_nulls_it(): + """Mirrors fieldcheck's test_na_outside_vocabulary_is_still_a_null_marker.""" + out = fd.clean( + _frame(), + verbose=False, + semantic_context={"columns": {"country": {"allowed_values": ["US", "GB", "FR"]}}}, + ) + assert pd.isna(out["country"].iloc[8]) + + +def test_the_exemption_is_scoped_to_the_declaring_column(): + """Declaring NA for one column must not rescue it everywhere.""" + df = pd.DataFrame( + { + "country": ["US", "NA", "DE", "FR", "GB", "JP", "CA", "AU", "BR"], + "note": ["a", "NA", "c", "d", "e", "f", "g", "h", "i"], + } + ) + out = fd.clean( + df, + verbose=False, + semantic_context={"columns": {"country": {"allowed_values": ISO}}}, + ) + assert out["country"].iloc[1] == "NA" + assert pd.isna(out["note"].iloc[1]) + + +@pytest.mark.parametrize("declared", ["na", "Na", " NA "]) +def test_vocabulary_matching_is_casefolded_and_trimmed(declared): + """Consistent with fieldcheck and with extra_sentinels normalisation.""" + out = fd.clean( + _frame(), + verbose=False, + semantic_context={"columns": {"country": {"allowed_values": ["US", declared]}}}, + ) + assert out["country"].iloc[8] == "NA" + + +@pytest.mark.parametrize("allowed", [None, [], "NA", 42, {"nested": "dict"}]) +def test_a_malformed_vocabulary_is_ignored_not_fatal(allowed): + """A bad hint must not crash cleaning, and must not grant an exemption. + + A bare string is rejected deliberately: iterating it would treat 'N' and + 'A' as separate permitted values. + """ + out = fd.clean( + _frame(), + verbose=False, + semantic_context={"columns": {"country": {"allowed_values": allowed}}}, + ) + assert pd.isna(out["country"].iloc[8]) + + +def test_the_none_brand_case_behaves_the_same_way(): + """'None' is a real brand; the same declaration rescues it.""" + df = pd.DataFrame( + { + "k": [f"r{i}" for i in range(9)], + "brand": [ + "Acme", + "Globex", + "Initech", + "Stark", + "Wayne", + "Hooli", + "Umbrella", + "Vandelay", + "None", + ], + } + ) + plain = fd.clean(df, verbose=False) + assert pd.isna(plain["brand"].iloc[8]) + + declared = fd.clean( + df, + verbose=False, + semantic_context={"columns": {"brand": {"allowed_values": [*df["brand"].tolist()]}}}, + ) + assert declared["brand"].iloc[8] == "None"