fix(generate): fail loudly on a headerless .wgt.csv source, don't misparse it#194
Merged
Conversation
…parse it <generate source="x.wgt.csv"> (the headerless value|weight format used by <key source=...>) fell through to the plain CSV reader, which has no coherent way to read it: it treats the first data row as a header, producing nonsense column names and one fewer row than the file actually has. Confirmed empirically - not a hypothetical. Scoped down from full <generate>-level weighted-entity support: wiring WeightedEntityDataSource into the chunked pagination path (ChunkSourceReader) properly is real surgery on shared, multiprocess-sensitive code - the "loads_all" branch it would need to reuse re-permutes its pool via get_distributed_data/get_unique_data, which would double-apply selection on top of already-weighted draws, and a naive per-page rebuild risks silently duplicating rows across workers. That's follow-up work, not this fix. ".wgt.ent.csv" is a normal headered CSV that happens to carry an extra "weight" column - reading it plainly (no resampling, weight column included as literal data) is coherent, so it's explicitly NOT touched by this guard. An existing test (test_source_script/test_iterate_source_scripted.xml) already relies on exactly that read for an unrelated feature (source-script CSV cell evaluation) and must keep working - verified it still passes. TDD: wrote failing tests for both the .wgt.csv-must-raise case and the .wgt.ent.csv-must-still-read-plainly case, confirmed RED/GREEN against each. Full suite 1428 passed/15 skipped, mypy/ruff clean.
|
4 tasks
ake2l
added a commit
that referenced
this pull request
Jul 6, 2026
…evel (#197) ## Summary - #194 blocked ALL \`.wgt.csv\` at \`<generate>\`-level, merged before #196 (\`.wgt.csv\` accepts an optional header) landed. That makes it too strict now: a HEADERED \`.wgt.csv\` reads coherently through the plain CSV path (proper column names, just unweighted) - the same tolerance \`.wgt.ent.csv\` already gets. Only a genuinely headerless \`.wgt.csv\` has no coherent plain-CSV reading at all (first data row misread as a header, producing nonsense column names). - Narrowed the guard to sniff the file's own first row (same check \`FileUtil.read_weight_csv\` itself uses to auto-detect a header) and only raise when there is no header to fall back on. ## Test plan - [x] Headerless \`.wgt.csv\` at \`<generate>\`-level still raises. - [x] New: headered \`.wgt.csv\` at \`<generate>\`-level now reads through (unweighted, coherent columns) instead of raising. - [x] \`.wgt.ent.csv\` regression test still passes. - [x] Full suite: 1442 passed, 15 skipped, mypy/ruff clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<key source="x.wgt.csv">(attribute level) already applies weights correctly (verified statistically: 79% vs 80% expected on a 3000-row sample). The real, confirmed bug is the opposite end:<generate source="x.wgt.csv">(entity level) silently misparses the headerless value|weight format as a headered CSV, producing nonsense column names and fewer rows than the file has.WeightedEntityDataSourceinto<generate>'s chunked pagination) is bigger and riskier than it first looks:ChunkSourceReader'sloads_allpath (what a weighted source would need) re-permutes its pool viaget_distributed_data/get_unique_data- reusing it would double-apply selection on top of already-weighted draws, and a naive per-page rebuild risks silently duplicating rows across multiprocess workers. That's real surgery on shared pagination code, scoped as follow-up work, not this PR..wgt.csvat<generate>-level..wgt.ent.csv(a normal headered CSV with an extra "weight" column) is explicitly NOT touched - reading it plainly (no resampling, weight column as literal data) is coherent and an existing test (test_source_script/test_iterate_source_scripted.xml) already relies on exactly that for an unrelated feature. Confirmed it still passes.Test plan
.wgt.csv-must-raise and.wgt.ent.csv-must-still-read-plainly, confirmed RED/GREEN.test_iterate_source_script(which reads a.wgt.ent.csv-named file for an unrelated reason) still passes.