diff --git a/src/freshdata/engine/context.py b/src/freshdata/engine/context.py index 07da0f15..864237c3 100644 --- a/src/freshdata/engine/context.py +++ b/src/freshdata/engine/context.py @@ -31,6 +31,11 @@ MIN_ROWS_FOR_ENGINE = 30 _ID_NAME = re.compile(r"(?:^|[_\s])(?:id|uuid|guid|key)s?$|^(?:id|index|pk)$", re.I) +_MONEY_NAME = re.compile( + r"price|amount|cost|salary|revenue|fee|balance|payment|charge|wage|income|" + r"spend|budget|paid|total|usd|eur|inr|gbp|cash|fare", + re.I, +) _TARGET_NAMES = frozenset({"target", "label", "y", "outcome", "class", "response"}) _TARGET_EXACT = frozenset({ "aqi", "score", "rating", "churn", "default", "conversion", "label", @@ -158,7 +163,12 @@ def infer_role( # Free text first: all-unique multi-word strings are prose, not keys. if _looks_like_text(s, nunique, non_null): return "text" - if nunique is not None and non_null >= 20 and nunique == non_null: + if ( + nunique is not None + and non_null >= 20 + and nunique == non_null + and not _MONEY_NAME.search(label) + ): return "id" return "categorical" # Mixed/object payloads we cannot reason about: treat as text (hands off). diff --git a/src/freshdata/semantic/context.py b/src/freshdata/semantic/context.py index 3aaf4a74..207c19c8 100644 --- a/src/freshdata/semantic/context.py +++ b/src/freshdata/semantic/context.py @@ -81,6 +81,7 @@ def _build_info( currencies: tuple[str, ...] = (), n_nonnull_override: int | None = None, ) -> SemanticColumnInfo: + """Build column semantic metadata from engine role and value distributions.""" name = str(col) series = df[col] # On the native distinct path *series* holds only distinct values, so the @@ -148,7 +149,11 @@ def _build_info( and ( ctx.role == "id" or column_name_is_identifier(name) - or (ctx.high_cardinality and ctx.role in ("text", "categorical")) + or ( + ctx.high_cardinality + and ctx.role in ("text", "categorical") + and not bool(_MONEY_NAME.search(name)) + ) ) ) ) @@ -173,8 +178,7 @@ def _build_info( ) ) money_like = ( - not free_text - and not identifier_like + not identifier_like and ( semantic_type in ("money", "currency") or bool(_MONEY_NAME.search(name)) diff --git a/src/freshdata/semantic/experts.py b/src/freshdata/semantic/experts.py index c5b1fff8..80f0b455 100644 --- a/src/freshdata/semantic/experts.py +++ b/src/freshdata/semantic/experts.py @@ -113,6 +113,7 @@ def parse_number_words(text: str) -> int | None: #: consults this table. Deliberately small: a currency absent from it and not #: settled by structure is reported ambiguous rather than guessed. _COMMA_DECIMAL_CURRENCIES = frozenset({"EUR", "CHF"}) +_ACCOUNTING_NUM_RE = re.compile(r"^[+-]?\s*\d[\d\s.,']*$") def _valid_grouping(part: str, sep: str) -> bool: @@ -215,17 +216,34 @@ def _split_amount(body: str, code: str | None) -> tuple[float | None, bool]: def parse_currency_parts(text: str) -> tuple[float | None, bool]: """``(value, ambiguous)`` for a currency string. - Requires an explicit currency marker (symbol or ISO-ish code) so that a bare - ``"1,200"`` is left to ordinary dtype repair, not treated as money. + Requires an explicit currency marker (symbol or ISO-ish code), or an + accounting-negative parenthesized amount with valid accounting punctuation, + so that bare numbers and unit strings are left to ordinary repair. """ s = text.strip() - has_symbol = any(c in s for c in _CURRENCY_SYMBOLS) + accounting_negative = s.startswith("(") and s.endswith(")") + if accounting_negative: + s = s[1:-1].strip() + if not s: + return None, False codes = {t.lower() for t in re.findall(r"[A-Za-z]+", s)} + if codes - _CURRENCY_CODES: + return None, False + has_symbol = any(c in s for c in _CURRENCY_SYMBOLS) has_code = bool(codes & _CURRENCY_CODES) - if not (has_symbol or has_code): + if not (has_symbol or has_code or accounting_negative): + return None, False + if ( + accounting_negative + and not (has_symbol or has_code) + and (not ("." in s or "," in s) or not _ACCOUNTING_NUM_RE.match(s)) + ): return None, False body = re.sub(r"[A-Za-z$€£¥₹\s\u00a0\u202f']", "", s) - return _split_amount(body, detect_currency(s)) + value, ambiguous = _split_amount(body, detect_currency(s)) + if value is not None and accounting_negative: + value = -abs(value) if value != 0 else 0.0 + return value, ambiguous def parse_currency(text: str) -> float | None: @@ -597,14 +615,16 @@ class CurrencyStringExpert: issue_type = "currency_string" def applies(self, info: SemanticColumnInfo) -> bool: + """True when the column has monetary semantic characteristics and is not free text.""" return info.money_like and not info.free_text def propose(self, series: pd.Series, info: SemanticColumnInfo) -> list[SemanticProposal]: + """Propose numeric conversions for formatted currency strings.""" out: list[SemanticProposal] = [] for raw, count in _value_counts(series).items(): if not isinstance(raw, str): continue - value = parse_currency(raw) + value, ambiguous = parse_currency_parts(raw) if value is None: continue code = detect_currency(raw) @@ -645,6 +665,36 @@ def propose(self, series: pd.Series, info: SemanticColumnInfo) -> list[SemanticP ) ) continue + if ambiguous: + out.append( + make_proposal( + column=info.name, + raw_value=raw, + proposed_value=value, + issue_type=self.issue_type, + expert=self.name, + base_confidence=0.60, + evidence=( + SemanticEvidence( + "pattern", f"{raw!r} is an ambiguous currency amount", 0.0 + ), + SemanticEvidence( + "context_hint", + f"{raw!r} has ambiguous thousands/decimal punctuation; " + "needs human review", + 0.0, + ), + ), + count=int(count), + rationale=( + f"{raw!r} is ambiguous between thousands and decimal separator; " + "needs human review" + ), + info=info, + risk_override="high", + ) + ) + continue evidence = ( SemanticEvidence("pattern", f"{raw!r} is a currency string", 0.0), SemanticEvidence("column_role", "column reads as monetary", 0.02), diff --git a/tests/expectations.py b/tests/expectations.py index 2cdd2464..d912a977 100644 --- a/tests/expectations.py +++ b/tests/expectations.py @@ -39,6 +39,7 @@ "large_panel", "duplicate_heavy", "locale_numbers", + "financial_ledger", "mixed_roles", ] diff --git a/tests/fixtures/financial_ledger.csv b/tests/fixtures/financial_ledger.csv new file mode 100644 index 00000000..8d6c5715 --- /dev/null +++ b/tests/fixtures/financial_ledger.csv @@ -0,0 +1,26 @@ +transaction_id,transaction_date,description,amount,currency,account +TX-001,2026-01-02,Opening balance,"$1,250.00",USD,Cash +TX-002,2026-01-03,Vendor invoice,"($1,250.00)",USD,Accounts Payable +TX-003,2026-01-04,European sale,"EUR 1.250,50",EUR,Revenue +TX-004,2026-01-05,European refund,"(EUR 1.250,50)",EUR,Refunds +TX-005,2026-01-06,Small card fee," € 0,50 ",EUR,Bank Fees +TX-006,2026-01-07,Service charge,"EUR 12,5",EUR,Bank Fees +TX-007,2026-01-08,Payroll batch,"$2,500.00",USD,Payroll +TX-008,2026-01-09,Payroll reversal," ($2,500.00) ",USD,Payroll +TX-009,2026-01-10,Office rent,"EUR 2.100,00",EUR,Operating Expense +TX-010,2026-01-11,Rent correction,"(EUR 2.100,00)",EUR,Operating Expense +TX-011,2026-01-12,Cloud hosting,"$3,450.75",USD,Technology +TX-012,2026-01-13,Cloud credit,"($3,450.75)",USD,Technology +TX-013,2026-01-14,Consulting income,"EUR 4.750,25",EUR,Revenue +TX-014,2026-01-15,Consulting reversal,"(EUR 4.750,25)",EUR,Revenue +TX-015,2026-01-16,Travel advance," $875.00 ",USD,Travel +TX-016,2026-01-17,Travel return," ($875.00) ",USD,Travel +TX-017,2026-01-18,Equipment purchase,"$12,000.00",USD,Fixed Assets +TX-018,2026-01-19,Equipment refund,"($1,200.00)",USD,Fixed Assets +TX-019,2026-01-20,Insurance premium,"EUR 1.050,00",EUR,Insurance +TX-020,2026-01-21,Insurance rebate,"(EUR 150,00)",EUR,Insurance +TX-021,2026-01-22,Interest received,"$45.67",USD,Interest +TX-022,2026-01-23,Interest correction,"($5.67)",USD,Interest +TX-023,2026-01-24,Settlement received,"EUR 10.000,00",EUR,Settlements +TX-024,2026-01-25,Settlement adjustment,"(EUR 250,00)",EUR,Settlements +TX-025,2026-01-26,Unmarked accounting adjustment,"(1,250.00)",USD,Settlements \ No newline at end of file diff --git a/tests/fixtures/financial_ledger.expectations.json b/tests/fixtures/financial_ledger.expectations.json new file mode 100644 index 00000000..8e573de0 --- /dev/null +++ b/tests/fixtures/financial_ledger.expectations.json @@ -0,0 +1,24 @@ +{ + "balanced": { + "idempotent": true, + "row_count": 25 + }, + "semantic_auto": { + "required_conversions": { + "amount": "float64", + "transaction_date": "datetime64" + }, + "target_values": { + "TX-002": -1250.0, + "TX-003": 1250.5, + "TX-004": -1250.5, + "TX-005": 0.5, + "TX-008": -2500.0, + "TX-023": 10000.0, + "TX-024": -250.0, + "TX-025": -1250.0 + }, + "row_count": 25, + "idempotent": true + } +} \ No newline at end of file diff --git a/tests/fixtures/golden/financial_ledger.balanced.report.json b/tests/fixtures/golden/financial_ledger.balanced.report.json new file mode 100644 index 00000000..a7090258 --- /dev/null +++ b/tests/fixtures/golden/financial_ledger.balanced.report.json @@ -0,0 +1,45 @@ +{ + "actions": [ + { + "column": "amount", + "confidence": 1.0, + "count": 4, + "description": "trimmed surrounding whitespace", + "human_review": false, + "memory_influenced": false, + "model_id": "", + "rationale": "", + "reversible": null, + "risk": "low", + "status": "automatic", + "step": "strip_whitespace" + }, + { + "column": "transaction_date", + "confidence": 1.0, + "count": 25, + "description": "converted to datetime64[ns]", + "human_review": false, + "memory_influenced": false, + "model_id": "", + "rationale": "", + "reversible": null, + "risk": "low", + "status": "automatic", + "step": "fix_dtypes" + } + ], + "cols_after": 6, + "cols_before": 6, + "columns_dropped": [], + "columns_imputed": [], + "columns_preserved": [], + "duplicates_removed": 0, + "missing_after": 0, + "missing_before": 0, + "outliers_handled": 0, + "recommendations": [], + "rows_after": 25, + "rows_before": 25, + "warnings": [] +} diff --git a/tests/fixtures/golden_diff_summary.jsonl b/tests/fixtures/golden_diff_summary.jsonl index 19bec2df..fc028b72 100644 --- a/tests/fixtures/golden_diff_summary.jsonl +++ b/tests/fixtures/golden_diff_summary.jsonl @@ -41,3 +41,4 @@ {"changed": false, "created": false, "fixture": "weather_json", "new_action_count": 3, "online": true, "previous_action_count": 3, "strategy": "balanced"} {"changed": true, "created": false, "fixture": "wine_quality", "new_action_count": 13, "online": true, "previous_action_count": 13, "strategy": "balanced"} {"changed": true, "created": false, "fixture": "adult_income", "new_action_count": 11, "online": true, "previous_action_count": 10, "strategy": "balanced"} +{"changed": true, "created": true, "fixture": "financial_ledger", "new_action_count": 2, "online": false, "previous_action_count": 0, "strategy": "balanced"} diff --git a/tests/test_currency_locale.py b/tests/test_currency_locale.py index a76afa4b..1dc344d1 100644 --- a/tests/test_currency_locale.py +++ b/tests/test_currency_locale.py @@ -11,11 +11,23 @@ from __future__ import annotations +import json +from pathlib import Path + import pandas as pd import pytest import freshdata as fd -from freshdata.semantic.experts import parse_currency, parse_currency_parts +from freshdata.config import CleanConfig +from freshdata.engine.context import infer_role +from freshdata.semantic.experts import ( + CurrencyStringExpert, + parse_currency, + parse_currency_parts, +) +from freshdata.semantic.types import SemanticColumnInfo + +FIXTURES_DIR = Path(__file__).parent / "fixtures" # -- European formats are no longer read as US ------------------------------ @@ -113,3 +125,122 @@ def test_clean_does_not_scale_european_amounts(text, expected): ) out = fd.clean(df, verbose=False, semantic_mode="auto") assert out["amount"].iloc[8] == expected + + +def test_clean_financial_ledger_fixture_respects_locale_and_accounting_values(): + """Verify financial ledger cleaning preserves row counts, dtypes, and values.""" + fixture = pd.read_csv(FIXTURES_DIR / "financial_ledger.csv") + expectations = json.loads( + (FIXTURES_DIR / "financial_ledger.expectations.json").read_text() + )["semantic_auto"] + + cleaned = fd.clean( + fixture, strategy="balanced", semantic_mode="auto", verbose=False + ) + + assert len(cleaned) == expectations["row_count"] + for column, dtype in expectations["required_conversions"].items(): + assert str(cleaned[column].dtype).startswith(dtype) + for transaction_id, expected in expectations["target_values"].items(): + actual = cleaned.loc[cleaned["transaction_id"] == transaction_id, "amount"] + assert len(actual) == 1 + assert actual.iloc[0] == expected + + +# -- PR #503 review fixes verification -------------------------------------- + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + ("(-$1,250.00)", -1250.0), + ("($-1,250.00)", -1250.0), + ("(-EUR 500.00)", -500.0), + ("(-10.50)", -10.50), + ], +) +def test_accounting_negatives_preserve_existing_negative_sign(text, expected): + """An explicit minus sign inside accounting parentheses must not flip positive.""" + val, _ = parse_currency_parts(text) + assert val == expected + + +@pytest.mark.parametrize( + "text", + [ + "(10 kg)", + "(page 3)", + "(see item 42)", + "(10)", + "(100)", + "(10%)", + "(10 / 20)", + "( - )", + ], +) +def test_unit_and_word_strings_in_parentheses_are_not_currency(text): + """Parenthesized units and citations must not be treated as negative currency.""" + val, ambiguous = parse_currency_parts(text) + assert val is None + assert ambiguous is False + + +def test_unmarked_parenthetical_with_ambiguous_separators_flagged(): + """Unmarked numbers like (1,250) must be flagged ambiguous and routed to review.""" + val, ambiguous = parse_currency_parts("(1,250)") + assert val == -1250.0 + assert ambiguous is True + + val2, ambiguous2 = parse_currency_parts("(1.250)") + assert val2 == -1.25 + assert ambiguous2 is True + + expert = CurrencyStringExpert() + info = SemanticColumnInfo( + name="amount", + role="numeric", + n_nonnull=1, + nunique=1, + high_cardinality=False, + preserve=False, + free_text=False, + numeric_like=True, + boolean_like=False, + money_like=True, + unit_like=False, + identifier_like=False, + ) + series = pd.Series(["(1,250)"]) + proposals = expert.propose(series, info) + assert len(proposals) == 1 + assert proposals[0].risk == "high" + assert proposals[0].confidence <= 0.60 + + +def test_payment_id_retains_identifier_role(): + """Names matching _ID_NAME must retain id role even when matching _MONEY_NAME.""" + cfg = CleanConfig() + # Repeating values (nunique != non_null) ensure role is not inferred purely by cardinality + for name in ("payment_id", "charge_id", "fee_id", "payment_key", "balance_uuid"): + series = pd.Series(["ID1", "ID1", "ID2"]) + assert infer_role(name, series, cfg) == "id" + + +def test_free_text_monetary_columns_stay_protected(): + """Free-text columns marked money_like must stay protected from currency conversion.""" + expert = CurrencyStringExpert() + info = SemanticColumnInfo( + name="notes", + role="text", + n_nonnull=3, + nunique=3, + high_cardinality=False, + preserve=False, + free_text=True, + numeric_like=False, + boolean_like=False, + money_like=True, + unit_like=False, + identifier_like=False, + ) + assert expert.applies(info) is False