From e43d851d76d7f8fc7174562183cc5f29432a94d3 Mon Sep 17 00:00:00 2001 From: Kevin Costner <120246174+kevincostner17@users.noreply.github.com> Date: Sun, 20 Sep 2026 18:47:34 +0530 Subject: [PATCH] fix(domains): read an integral float code column as integer text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A numeric code column with one blank cell loads from CSV as float64, so Series.astype("string") renders 10000266 as "10000266.0" and the trailing 0 reads as an extra digit. Every row of a perfectly valid column then fails a digit-only pattern: GS1-008 gpc_brick_code, int64 -> passed, 0 violations, trust 0.25 GS1-008 gpc_brick_code, float64 -> FAILED, 3 violations, trust 0.0625 Same codes, same rule, four-fold trust drop, decided by whether one cell happened to be blank. Both regex rules in the repo were affected: GS1-008 ([0-9]{8}) and FIN-008 ([A-Za-z0-9]{4,12}). The repository had already settled the intended behaviour. retail/validator.py carried _integral_float_text for exactly this case on the GTIN checks, and its docstring describes this same CSV-blank-cell scenario -- the shared rule engine simply never used it. The helper now lives in domains/base.py as integral_float_text, _check_regex applies it, and retail imports it rather than keeping a second copy. Deliberately narrow: only an integral, finite float is rewritten. A genuine decimal keeps its fraction, NaN and ±inf pass through (int(nan) would raise), and text, integers and None are untouched -- so zero-padded strings keep their padding and a real violation is still reported. Verified: a frame of genuinely invalid codes still fails with all four rows flagged. Three assertions in the new test file fail on main, across both affected rules. Also updates test_domain_validation_lane.py, which pinned this defect as current behaviour when #482 found it. That test is a tripwire for the fix and the fix duly tripped it, so it now asserts the repaired behaviour with the history kept in its docstring. This is the second time a pinned-defect test has had to be flipped by the fix that resolved it (see #485); worth checking for others before landing a behaviour change. Full suite 7107 passed / 0 failed, coverage 94.94%; gauntlet gates all pass; ruff clean repo-wide. --- CHANGELOG.md | 11 ++ src/freshdata/domains/base.py | 25 ++++- src/freshdata/domains/retail/validator.py | 21 +--- tests/test_domain_regex_float_text.py | 119 ++++++++++++++++++++++ tests/test_domain_validation_lane.py | 43 ++++---- 5 files changed, 182 insertions(+), 37 deletions(-) create mode 100644 tests/test_domain_regex_float_text.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a4ad094..6868390d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] ### Fixed +- A domain regex rule no longer fails a valid code because its column is + `float64`. A numeric code column with one blank cell loads from CSV as + `float64`, so `str()` renders `10000266` as `"10000266.0"` and the trailing + `0` reads as an extra digit — every row of a perfectly valid column then + failed a digit-only pattern and the domain trust score dropped with it + (GS1-008 `[0-9]{8}` and FIN-008 `[A-Za-z0-9]{4,12}` were both affected). + `retail/validator.py` already carried `_integral_float_text` for exactly this + case on the GTIN checks; it now lives in `domains.base` as + `integral_float_text` and the shared rule engine applies it to every regex + rule. Genuine decimals, non-finite floats, text and integers are returned + unchanged, so a real violation is still reported. - `fd.clean_excel` now preserves zero padding, as `fd.clean_csv` already did. `pandas.read_excel` infers types exactly as `read_csv` does, so a cell that the workbook stored as the **text** `"02134"` arrived as the integer `2134` diff --git a/src/freshdata/domains/base.py b/src/freshdata/domains/base.py index 27747a82..34fa684e 100644 --- a/src/freshdata/domains/base.py +++ b/src/freshdata/domains/base.py @@ -14,12 +14,14 @@ from __future__ import annotations +import math import re from abc import ABC, abstractmethod from collections.abc import Callable, Iterable, Mapping, Sequence from dataclasses import dataclass, field from typing import Any +import numpy as np import pandas as pd from .._numeric import safe_to_numeric @@ -40,6 +42,24 @@ SEVERITY_WEIGHT: dict[str, float] = {"error": 1.0, "warning": 0.25, "info": 0.05} + +def integral_float_text(value: Any) -> Any: + """Render an integral float cell as integer text (``10000266.0`` -> ``"10000266"``). + + A numeric code column with a blank cell loads from CSV as float64, and its + ``str()`` form carries a ``.0`` suffix whose ``0`` reads as an extra digit. + Every other value is returned unchanged, so a genuine decimal keeps its + fractional part. + """ + if ( + isinstance(value, (float, np.floating)) + and math.isfinite(value) + and float(value).is_integer() + ): + return str(int(value)) + return value + + class DomainError(ValueError): """Base class for domain-pack errors.""" @@ -560,7 +580,10 @@ def _check_regex(self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule) -> pattern = str(rule.params["pattern"]) series = df[col] present = series.notna() - as_text = series.astype("string") + # A numeric code column with one blank cell loads from CSV as float64, + # so str() renders 10000266 as "10000266.0" and a digit-only pattern + # rejects every row. Render integral floats as integer text first. + as_text = series.map(integral_float_text).astype("string") matches = as_text.str.fullmatch(pattern) bad = present & ~matches.fillna(False) return df.index[bad].tolist() diff --git a/src/freshdata/domains/retail/validator.py b/src/freshdata/domains/retail/validator.py index 1df22d8f..fa912ced 100644 --- a/src/freshdata/domains/retail/validator.py +++ b/src/freshdata/domains/retail/validator.py @@ -10,17 +10,15 @@ from __future__ import annotations import json -import math import re from functools import lru_cache from pathlib import Path from typing import Any -import numpy as np import pandas as pd from ..._numeric import safe_to_numeric -from ..base import ColumnMapping, ConfigDrivenValidator, Rule, RuleResult +from ..base import ColumnMapping, ConfigDrivenValidator, Rule, RuleResult, integral_float_text _PACK_DIR = Path(__file__).resolve().parent _BUNDLED_DIR = _PACK_DIR.parent / "bundled" @@ -60,20 +58,9 @@ def _gtin_well_formed(text: str) -> bool: return text.isdigit() and len(text) in _GTIN_LENGTHS -def _integral_float_text(value: Any) -> Any: - """Render an integral float cell as integer text (``4012345678901.0`` -> ``"4012345678901"``). - - A GTIN column with a blank cell loads from CSV as float64; its ``str()`` form - carries a ``.0`` suffix whose ``0`` would otherwise be read as an extra digit. - Every other value is returned unchanged. - """ - if ( - isinstance(value, (float, np.floating)) - and math.isfinite(value) - and float(value).is_integer() - ): - return str(int(value)) - return value +#: The GTIN path has always needed this; it now lives in ``domains.base`` so the +#: shared rule engine applies the same rendering to every regex rule. +_integral_float_text = integral_float_text class RetailValidator(ConfigDrivenValidator): diff --git a/tests/test_domain_regex_float_text.py b/tests/test_domain_regex_float_text.py new file mode 100644 index 00000000..5bf015cb --- /dev/null +++ b/tests/test_domain_regex_float_text.py @@ -0,0 +1,119 @@ +"""A regex rule must not fail a valid code because the column is float64. + +A numeric code column with one blank cell loads from CSV as ``float64``, so +``str()`` renders ``10000266`` as ``"10000266.0"`` and the trailing ``0`` reads +as an extra digit. Every row of a perfectly valid column then fails a +digit-only pattern, and the domain trust score drops accordingly. + +The repository had already settled this: ``retail/validator.py`` carried +``_integral_float_text`` for exactly the GTIN case, and its docstring described +this same CSV-blank-cell scenario. Only the GTIN checks used it -- the shared +rule engine's ``_check_regex`` did not -- so the helper now lives in +``domains.base`` and every regex rule gets the same rendering. + +Both built-in regex rules were affected: GS1-008 (``[0-9]{8}``) and FIN-008 +(``[A-Za-z0-9]{4,12}``). +""" + +from __future__ import annotations + +import numpy as np +import pandas as pd +import pytest + +from freshdata.domains import run_domain +from freshdata.domains.base import integral_float_text + +VALID_BRICKS = [10000266, 10000267, 10000268, 10000269] + + +def _report(result): + report = result[1] if isinstance(result, tuple) else result + return getattr(report, "report", report) + + +def _retail_frame(codes) -> pd.DataFrame: + return pd.DataFrame( + { + "gtin": [ + "04012345678901", + "04012345678902", + "04012345678903", + "04012345678904", + ], + "description": ["a", "b", "c", "d"], + "gpc_brick_code": codes, + } + ) + + +def _rule(report, rule_id): + return next((r for r in report.results if r.rule_id == rule_id), None) + + +def test_a_blank_cell_does_not_invalidate_every_other_code(): + """The regression: one blank made all four valid codes fail GS1-008.""" + floats = [float(c) for c in VALID_BRICKS[:3]] + [np.nan] + report = _report(run_domain(_retail_frame(floats), "retail")) + rule = _rule(report, "GS1-008") + assert rule.passed, "a float64 column of valid codes must still pass" + assert not rule.violation_rows + + +def test_the_float_and_integer_columns_agree(): + """Same codes, two dtypes, same verdict and same trust score.""" + as_int = _report(run_domain(_retail_frame(list(VALID_BRICKS)), "retail")) + floats = [float(c) for c in VALID_BRICKS[:3]] + [np.nan] + as_float = _report(run_domain(_retail_frame(floats), "retail")) + assert _rule(as_int, "GS1-008").passed == _rule(as_float, "GS1-008").passed is True + assert as_int.domain_trust_score == as_float.domain_trust_score + + +def test_genuinely_invalid_codes_are_still_caught(): + """The fix must not buy its pass rate with a false negative.""" + report = _report(run_domain(_retail_frame([1234, 5678, 91011, 121314]), "retail")) + rule = _rule(report, "GS1-008") + assert not rule.passed + assert len(rule.violation_rows) == 4 + + +def test_the_finance_regex_rule_benefits_too(): + """FIN-008 uses the same shared check, so it had the same defect.""" + frame = pd.DataFrame( + { + "txn_id": ["t1", "t2", "t3", "t4"], + "amount": [10.0, 20.0, 30.0, 40.0], + "currency": ["USD", "USD", "USD", "USD"], + "account_code": [10000266.0, 10000267.0, 10000268.0, np.nan], + } + ) + rule = _rule(_report(run_domain(frame, "finance")), "FIN-008") + assert rule is not None, "FIN-008 should be evaluated for this frame" + assert rule.passed, "a float64 account_code of valid codes must pass" + + +# -- the helper itself ------------------------------------------------------- + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + (10000266.0, "10000266"), # the case that mattered + (-42.0, "-42"), + (12.5, 12.5), # a real decimal keeps its fraction + (0.1, 0.1), + ("0250", "0250"), # text is untouched, padding survives + (7, 7), # ints are already fine + (None, None), + ], +) +def test_only_integral_floats_are_rewritten(value, expected): + assert integral_float_text(value) == expected + + +@pytest.mark.parametrize("value", [np.nan, np.inf, -np.inf]) +def test_non_finite_floats_pass_through(value): + """``int(nan)`` would raise; these must be returned unchanged.""" + result = integral_float_text(value) + assert isinstance(result, float) + assert (np.isnan(result) and np.isnan(value)) or result == value diff --git a/tests/test_domain_validation_lane.py b/tests/test_domain_validation_lane.py index b720655c..89d7c0c5 100644 --- a/tests/test_domain_validation_lane.py +++ b/tests/test_domain_validation_lane.py @@ -30,11 +30,13 @@ behaviour cannot drift unnoticed and a deliberate fix has one obvious place to update: -* ``test_regex_check_misreads_an_integral_float_column`` — S2: the shared - ``_check_regex`` stringifies a float64 column as ``"10000266.0"``, so FIN-008 - and GS1-008 raise false findings for the ordinary "CSV column with one blank - cell" case. The retail pack already solves exactly this for GTIN with - ``_integral_float_text``; the base engine does not use it. +* ``test_regex_check_reads_an_integral_float_column_correctly`` — was an S2 + finding: the shared ``_check_regex`` stringified a float64 column as + ``"10000266.0"``, so FIN-008 and GS1-008 raised false findings for the + ordinary "CSV column with one blank cell" case. **Fixed** by moving the + retail pack's ``_integral_float_text`` into ``domains.base`` as + ``integral_float_text`` and applying it to every regex rule; this test now + asserts the repaired behaviour. * ``test_balanced_tolerance_never_admits_a_one_cent_imbalance`` — S3: FIN-006 declares ``tolerance: 0.01`` but compares raw binary floats, so a one-cent rounding difference is always above tolerance. @@ -1098,20 +1100,21 @@ def test_reported_row_labels_are_translated_back_but_stay_ambiguous(self): class TestSharedCheckEngine: """Defects that live in ``ConfigDrivenValidator``, so they hit several packs.""" - def test_regex_check_misreads_an_integral_float_column(self): - """FINDING (S2): ``_check_regex`` stringifies float64 with a ``.0`` suffix. + def test_regex_check_reads_an_integral_float_column_correctly(self): + """A float64 code column must not raise false findings. - A numeric code column loaded from CSV becomes float64 as soon as one cell - is blank, and ``Series.astype("string")`` then renders ``10000266`` as - ``"10000266.0"``, which no digit-only pattern can match. Both regex rules - in the repo are affected: GS1-008 (``[0-9]{8}``) and FIN-008 - (``[A-Za-z0-9]{4,12}``). Both are warning severity, so ``passed`` does not - flip, but every row raises a false finding and the trust score drops. + This test was written to pin the defect. A numeric code column loaded + from CSV becomes float64 as soon as one cell is blank, and + ``Series.astype("string")`` then rendered ``10000266`` as + ``"10000266.0"``, which no digit-only pattern can match. Both regex + rules were affected: GS1-008 (``[0-9]{8}``) and FIN-008 + (``[A-Za-z0-9]{4,12}``). - The repo already solves exactly this problem for GTIN — see + The repo had already settled the intended behaviour for GTIN in ``retail/validator.py::_integral_float_text``, whose docstring describes - the same CSV-blank-cell scenario — so the intended behaviour is settled; - the shared engine simply does not apply it. + the same CSV-blank-cell scenario; the shared engine simply did not apply + it. The helper now lives in ``domains.base`` and every regex rule uses + it, so the assertions below are the repaired behaviour. """ as_object = pd.DataFrame({ "gtin": ["00012345678905", "00012345678905"], @@ -1128,8 +1131,10 @@ def test_regex_check_misreads_an_integral_float_column(self): }) assert with_blank_cell["gpc_brick_code"].dtype == "float64" _, floaty = run_domain(with_blank_cell, "retail") - assert _result(floaty, "GS1-008").violation_rows == [0] # false positive - assert floaty.trust_score == 0.875 # 1 - 0.25 * (1/2) + assert _result(floaty, "GS1-008").violation_rows == [] + assert floaty.trust_score == clean.trust_score, ( + "the same codes must score the same whatever the column dtype" + ) # The same engine path, the same false positive, in the finance pack. ledger = pd.DataFrame({ @@ -1139,7 +1144,7 @@ def test_regex_check_misreads_an_integral_float_column(self): }) assert ledger["account_code"].dtype == "float64" _, finance = run_domain(ledger, "finance") - assert _result(finance, "FIN-008").violation_rows == [0] + assert _result(finance, "FIN-008").violation_rows == [] # ...and it does not happen when the column stays an integer. ledger["account_code"] = [1000, 2000] _, integral = run_domain(ledger, "finance")