Skip to content

fix(#645): to_parquet stops deleting real data via string-matched nulls#655

Merged
ligon merged 3 commits into
developmentfrom
fix/645-to-parquet-null-coercion
Jul 22, 2026
Merged

fix(#645): to_parquet stops deleting real data via string-matched nulls#655
ligon merged 3 commits into
developmentfrom
fix/645-to-parquet-null-coercion

Conversation

@ligon

@ligon ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Closes #645.

local_tools.to_parquet stringified every dtype == object column and then
tried to recover the nulls by string matching:

col.astype(str).astype('string[pyarrow]').replace({'nan': None, 'None': None, '<NA>': None})

After astype(str) a genuine missing and a legitimate value spelling 'None'
are the same characters, so the recovery cannot be right — and None was the
canonical library label for "no education"
(categorical_mapping/harmonize_education.org). Every such person was nulled
in the act of writing the cache, and _finalize_result's dropna(how='all')
then deleted the row outright: individual_education has exactly one column,
so a nulled value is a deleted person.

Recovered counts (cold, isolated LSMS_DATA_DIR, measured both directions)

country before (cold / warm) after recovered
Guatemala 20,678 / 20,678 29,527 +8,849 (30%)
Tajikistan 59,297 / 54,462 59,790 +493 / +5,328
Ethiopia 63,139 / 62,939 63,181 +42 / +242

Every recovered row is a person the survey recorded as having no education.
Ethiopia's pair is exactly the 63,139 vs 62,939 in the issue title — that
call-order symptom is gone.

Guatemala is why this survived so long: cold == warm == wrong, so no A/B
comparison could see it, and .coder/coverage/latest.csv certified it
Guatemala,individual_education,2000,sane,declared,20678. After the fix all
three are cold == warm, which is now a tested invariant.

What changed

  1. The write path (local_tools.py). Capture the null mask before the
    cast, restore it after — na = col.isna().mask(na). Only cells that
    were actually missing become null.
  2. LSMS_CACHE_SCHEMA 4 → 5. The loss happened in the act of writing, so
    every existing L2-wave and L2-country parquet already has the nulls baked in,
    and the per-table input hashes are unchanged. Without the bump nothing
    rebuilds and the fix is invisible on exactly the machines where the
    corruption is present. This forces one full rebuild.
  3. The canonical label NoneNo education (38 rows across 20
    harmonize_education tables, canonical_education_labels.org, 3 wave
    data_info.yml inline mappings, China/_/china.py,
    Uganda/_/_education_helpers.py). A canonical value that collides with the
    null literal under str(), YAML, CSV and parquet round-trips is a trap
    that keeps re-arming itself; the write-path fix alone leaves it loaded.
    None / NONE / none stay as accepted variants in the new
    data_info.yml spellings block, so historical caches and any
    not-yet-updated country still resolve at API time — no rebuild needed for
    the rename itself.
  4. dropna(how='all') now says how many rows it took (country.py,
    logger.info). This is where a value corruption upstream becomes a silent
    deletion; 30% of a table vanished here without a word. INFO, not a warning
    — legitimately hollow rows are common and it must not cry wolf. Whether it
    should escalate above some rate is left to you.

The pandas-3.0 assessment (issue asked; answer: KEEP the block)

The comment's premise — "Pyarrow can't deal with mixes of types in columns of
type object"
— is still true under pandas 3.0.2 / pyarrow 23.0.1.
Measured: str+int, str+float, str+Timestamp, str+list/tuple/dict,
bool+str, str+Decimal all still raise ArrowTypeError. Pinned by
test_mixed_type_object_column_is_still_stringified.

What has changed is that far fewer columns reach it: with
future.infer_string=True a pure string column is inferred as str, not
object, so it skips the guard entirely. That is why exactly 3 of 25
at-risk countries were hit — the other 22 were correct by accident of dtype
inference
, not by design.

Narrowing the block to genuinely-mixed columns was considered and rejected.
It would change the stored dtype of homogeneous non-str object columns (an
object column of ints would round-trip as int64 instead of the strings
'1','2'), i.e. it would silently change returned data across the corpus —
the exact class of failure this issue is about. Recorded in the ledger, not
acted on.

Blast radius — everything that changed behaviour

Swept 28 data-bearing countries cold (one process, one empty
LSMS_DATA_DIR each, only dvc-cache symlinked), plus before/after cold
builds of the other tables that could manufacture these literals.

  • individual_education, 28 countries — only the 3 above change. All 28
    are cold == warm, and every value falls inside the canonical vocabulary.
  • Nigeria.food_acquired — the only other site deliberately manufacturing
    the literal: df['u'].fillna('None') in 4 waves' food_acquired.py.
    1,005,656 rows before and after (no rows gained or lost) and Quantity_kg
    identical; what changes is that 1,151 rows now carry the intended sentinel
    u='None' instead of a NaN index key. Strictly better (a NaN key is the
    GH bug: framework silently drops rows on duplicate canonical index (groupby().first()) #323 §3b delete-on-collapse hazard) but it is visible to anyone reading
    that level. Recommend renaming that sentinel too (e.g. Not recorded) —
    deliberately not bundled here, since u feeds the unit-conversion tables.
  • Guyana.housing / South Africa.housing — carry Toilet == 'None'
    (52 / 1,102 rows). Measured identical before and after: those columns
    arrive as str, not object, so the block never fired. No change.
  • Read-path NA mop-ups left alonetransformations.py _na_strings,
    country.py _NA_SENTINELS, conversion.py. All operate on Relationship
    / sex / age / dates, whose vocabularies genuinely exclude these strings.
    They are deliberate and correct; generalising one would recreate this bug a
    layer up.

One pre-existing defect surfaced (not caused by this PR)

Declaring the Educational Attainment vocabulary makes
diagnostics._check_declared_spellings enforce it. Auditing 107 freshly
cold-built parquets
(28 countries, wave- and country-level) turned up exactly
one off-vocabulary value in the whole corpus:
Ethiopia 2013-14 hh_s2q05 has one row of 12,583 carrying a bare 76.0
that never received a Stata value label. It cannot be handled in
categorical_mapping.org — that table's Original Label column is mixed text,
so every key is read as a string and a float source value can never match
(contrast Guyana, whose all-numeric table parses its keys as floats). Folded to
Unknown by a df_edit hook in 2013-14/_/mapping.py, reusing the shape of
Uganda's existing _/_education_helpers.py and reading the vocabulary from
data_info.yml rather than re-listing it. Unknown is not a guess:
canonical_education_labels.org defines it as the sentinel for "unmappable".

On the pre-#644 base this was cosmetic (the row was collapsed away before
reaching the API); on the current base it is not#644's re-key means
'76.0' now survives into the user-facing frame, so the hook is what keeps
Ethiopia's Educational Attainment inside its own declared vocabulary.
Row count is unaffected either way (63,181); Unknown goes 33 → 34.

Tests

tests/test_gh645_to_parquet_null_coercion.py — 17 tests, in three groups: the
write path (data-free), the vocabulary (data-free), and the three recovered row
counts (cold, requires_s3 from tests/conftest.py so the data-free CI job
skips rather than errors). The integration cases run in a subprocess with a
genuinely empty data root — LSMS_NO_CACHE=1 MASKS this bug (it skips the
L2-wave write where the destruction happened), so it is exactly the wrong lever.

Negative control: against unmodified origin/development, 14 of 17
fail
. The 3 that pass are the two deliberate "behaviour must NOT change"
guards, plus cold == warm for Guatemala — which passed because Guatemala was
equal and wrong, the whole reason nobody noticed.

Merged guards re-run and green: test_gh323_explicit_reducers.py,
test_gh323_grain_contract.py, test_schema_consistency.py,
test_canonical_spellings.py, test_cache_hash_invalidation.py (280 passed).
No aggregation: key, no reducer — GH #323 D1 respected.

Ledger: .coder/ledger/645-to-parquet-null-coercion.md.

For the reviewer

  • Row counts change, so anything published off Guatemala,
    Tajikistan or Ethiopia individual_education needs re-checking; Guatemala
    grows 43%.
  • .coder/coverage/latest.csv is now wrong for those cells and needs a
    make matrix refresh (not done here — it is a generated snapshot).
  • No cell is blessed in this PR: I read these numbers as row counts and label
    censuses, not as substantive education distributions.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr

ligon and others added 3 commits July 21, 2026 21:10
`to_parquet` serialized object columns as
`astype(str).astype('string[pyarrow]').replace({'nan': None, 'None': None,
'<NA>': None})` -- it stringified FIRST and then tried to recover the nulls by
matching the resulting characters.  After `astype(str)` a genuine missing and a
legitimate value spelling 'None' are the same characters, so the recovery is
wrong in principle.  `None` was the canonical library label for "no education",
so every never-schooled person was nulled IN THE ACT OF WRITING the cache, and
`_finalize_result`'s `dropna(how='all')` then deleted the row -- the table has
exactly one column, so a nulled value is a deleted person.

Measured cold, isolated LSMS_DATA_DIR, both directions:

  Guatemala   individual_education  20,678 / 20,678 -> 29,527  (+8,849, 30%)
  Tajikistan  individual_education  59,297 / 54,462 -> 59,790  (+493 / +5,328)
  Ethiopia    individual_education  63,139 / 62,939 -> 63,181  (+42 / +242)

Guatemala was cold == warm == wrong, so no A/B comparison could see it and the
coverage matrix graded it `sane`.  All three are now cold == warm, pinned as a
test.

Changes:

* local_tools.to_parquet -- capture the null mask BEFORE the cast, restore it
  after.  The pyarrow guard itself is KEPT: a genuinely mixed object column
  still raises ArrowTypeError under pandas 3.0.2 / pyarrow 23, and narrowing
  the block would change the stored dtype of homogeneous non-str object columns
  (silently changing returned data corpus-wide -- the very failure class this
  issue is about).

* LSMS_CACHE_SCHEMA 4 -> 5.  The loss happened in the act of writing, so every
  existing L2-wave and L2-country parquet already has the nulls baked in, and
  the per-table input hashes are unchanged.  Without the bump nothing rebuilds
  and the fix is invisible on exactly the machines where the corruption is
  present.  Forces one full rebuild; this changes returned data.

* Canonical label `None` -> `No education` (38 rows across 20
  harmonize_education tables, canonical_education_labels.org, 3 wave
  data_info.yml inline mappings, China/_/china.py,
  Uganda/_/_education_helpers.py).  A canonical value colliding with the null
  literal under str()/YAML/CSV/parquet keeps re-arming the trap.  `None` /
  `NONE` / `none` stay as accepted VARIANTS in a new data_info.yml `spellings`
  block, so historical caches and un-updated countries resolve at API time.

* Ethiopia 2013-14 `individual_education` df_edit hook: one row of 12,583
  carries a bare unlabelled float (76.0) that cannot be mapped in the org table
  (mixed-text Original Label => string keys can never match a float source).
  Folded onto `Unknown`, the vocabulary's own "unmappable" sentinel.  Zero
  effect on the row count; on the current base (post-#644) it is what keeps the
  API frame inside its own declared vocabulary.  Surfaced by
  declaring the vocabulary.

* country.py: `dropna(how='all')` now logs how many rows it removed.  This is
  where a value corruption becomes a silent deletion, and 30% of a table
  vanished here without a word.

Blast radius swept: 28 data-bearing countries cold (all cold == warm, all
in-vocabulary) + 107 freshly-built parquets audited against the declared
vocabulary (0 failures).  Nigeria/food_acquired is the one other behaviour
change: its deliberate `u = fillna('None')` sentinel now survives as the
literal instead of becoming a NaN index key -- same row count (1,005,656) and
identical Quantity_kg.  Guyana / South Africa `housing.Toilet == 'None'`
measured identical before and after (those columns arrive as `str`, so the
block never fired).

Tests: tests/test_gh645_to_parquet_null_coercion.py (17).  Negative control
against unmodified origin/development: 14 of 17 fail.  GH #323 guards re-run
green (no `aggregation:` key, no reducer).

Ledger: .coder/ledger/645-to-parquet-null-coercion.md

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
`pytest.raises(Exception)` would have passed on any error at all, including
one caused by the test's own frame construction -- which is exactly the trap
this file exists to guard against (an earlier draft of this very test built a
mis-indexed frame and silently asserted nothing).  Name the two errors pyarrow
actually raises for a mixed object column, and write into a TemporaryDirectory
instead of `tempfile.mktemp`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
First CI run on this PR failed at collection with BOTH import spellings:

  from tests.conftest import requires_s3  -> ModuleNotFoundError: 'tests.tests'
  from conftest import requires_s3        -> ImportError: cannot import name
                                            'requires_s3' from 'conftest'
                                            (<repo root>/conftest.py)

The repo has conftest.py at BOTH the root and in tests/, and under the CI
rootdir the root one wins the bare name while 'tests' resolves relative to
tests/ itself.  This is a live trap for #648's helper: tests/conftest.py's own
docstring recommends 'from conftest import aws_creds_available', which no
module had yet exercised and which does not work in CI.

Load it by file path instead — import-mode agnostic, and still #648's single
source of truth rather than a fourth private copy of the creds check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Note for review — a finding about #648, surfaced by this PR's first CI run.

tests/conftest.py is the single home for requires_s3 / aws_creds_available, and its own docstring recommends:

from conftest import aws_creds_available

That does not work in CI. This repo has a conftest.py at both the root and in tests/, and under the CI rootdir the root one wins the bare name — so the import resolves to the root conftest.py, which has no requires_s3. The obvious alternative fails too, because tests resolves relative to tests/ itself:

from tests.conftest import requires_s3  ->  ModuleNotFoundError: No module named 'tests.tests'
from conftest import requires_s3        ->  ImportError: cannot import name 'requires_s3'
                                            from 'conftest' (<repo root>/conftest.py)

No module had exercised the documented pattern yet, so it had never been caught. This PR works around it by loading tests/conftest.py by file path (import-mode agnostic), which keeps #648's single source of truth rather than adding a fourth private copy of the creds check.

The durable fix belongs in #648's own files, not here — either re-export requires_s3 / aws_creds_available from the root conftest.py, or correct the tests/conftest.py docstring to document the by-path load. I deliberately did not touch shared test infrastructure in a data-correctness PR; flagging it so the next module that reaches for the helper does not lose the same CI cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant