Repository navigation
fix: align validation results with input index - #1078
suryapilla wants to merge 1 commit into
Conversation
Signed-off-by: suryapilla <pslsubbarao@gmail.com>
|
All contributors have signed the DCO ✍️ ✅ |
[Medium impact] The PR appears safe to merge. SummaryKeeps validation results aligned with the input rows when their index has gaps or a different order.
Reviews (1) · Last reviewed commit: "fix: align validation results with input..." · Reviewed by Greptile |
|
I have read the DCO document and I hereby sign the DCO. |
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for the fix, @suryapilla!
Summary
Preserving the input index fixes validation-result alignment after upstream rows are dropped or reordered, matching the reported issue. The new total-count guard is useful, but count validation also needs to happen per batch to prevent incorrect assignments.
Findings
Warnings — Worth addressing
packages/data-designer-engine/src/data_designer/engine/column_generators/generators/validation.py:102 — Check each batch's result count before combining outputs
- What: The new guard checks only the combined count. For four inputs split into batches of two, responses containing one and three results still total four and pass the check. This behavior predates the PR; the request is to complete the count validation being added here.
- Why: Results can silently attach to the wrong records. In the reproduction, inputs with IDs
[0, 1, 2, 3]receive results for IDs[0, 2, 3, 3]despite the correct total count. - Suggestion: Validate each batch's result count against its input count before flattening, covering both sequential and parallel execution. Add a regression where short and excess responses cancel in the aggregate, and ensure malformed batches cannot shift another batch's results.
packages/data-designer-engine/tests/engine/column_generators/generators/test_validation.py:283 — Cover empty and excess validator results
- What: The mismatch test covers one returned result for three inputs, but does not cover empty or excess results.
- Why: A guard that rejects only short responses would still pass this test, leaving excess-result handling unprotected. The current production guard correctly handles these total-count cases.
- Suggestion: Parametrize the returned count over
0,1, and4for three input records, and assert the corresponding error message.
What Looks Good
- Preserving
data.indexfixes extra rows after upstream drops and incorrect assignments after reordering. - The regression tests verify per-row validation values; all three fail against the parent implementation and pass with this fix.
- The batching mock now returns one result per input record, matching the validator contract.
Residual Risk
All 740 tests across column generators, validators, and dataset builders passed, along with lint and formatting checks. Live remote services, the full repository suite, and the supported Python/pandas version matrix were not exercised.
Verdict
Needs changes — enforce per-batch result counts and add coverage for empty, excess, and compensating batch responses before merging.
This review was generated by an AI assistant.
📋 Summary
Fixes misaligned validation results when upstream rows were dropped or reordered.
ValidationColumnGeneratornow builds its results with the input frame's index, and raises a clear error if a validator returns a different number of results than records.🔗 Related Issue
Fixes #1073
🔄 Changes
index=data.index, so results stay aligned to their rows. Previously a fresh0..n-1index pluspd.concat(..., axis=1)produced extra rows when the input index had gaps (returned 4 rows but 3 were expected), and could attach results to the wrong rows when the index was reordered (as @ting-hong-shieh found on the issue).DataDesignerRuntimeErrorwhen a validator returns a different number of results than records. Previously this was silently padded with empty rows.test_validation_column_generator_generate_with_different_strategies: its mock returned one result per batch instead of one per record, and only passed because of the silent padding. It now returns one result per record, like real validators.🧪 Testing
make testpassesNew tests:
test_validation_column_generator_preserves_non_contiguous_index: fails onmain(4 rows for 3), passes with the fixtest_validation_column_generator_preserves_reordered_index: fails onmain(results on the wrong rows), passes with the fixtest_validation_column_generator_raises_on_result_count_mismatch: covers the new errormake check-all-fixpasses.✅ Checklist
triagedPrepared with AI assistance; I reviewed and tested the changes locally.