Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Per-file column sets can cause false additions and removals when column sets differ.
Pull request overview
Normalizes missing JSON fields before no-key content hashing, while preserving nested values and explicit-key behavior.
Changes:
- Fills missing fields with
nullbefore hashing. - Adds regression coverage for normalization, deduplication, explicit keys, and nested values.
File summaries
| File | Summary |
|---|---|
tests/test_json_normalization.py |
Adds coverage for JSON normalization behavior. |
csv_diff/__init__.py |
Normalizes missing fields before generating content keys. |
Review details
Suppressed comments (1)
csv_diff/init.py:42
- This normalizes against each file's own
common_keys, which changes no-key comparison semantics when the files have different column sets. For example, previous[{'id': 1}, {'a': 0}]and current[{'id': 1}, {'b': 0}]previously matched the first rows and reported only the column changes, but now those hashes include differenta: nullversusb: nullfields, so the same row is also reported as removed and added. Use a shared column set for hashing, or preserve the old hash path whenever the column unions differ.
for row in raw_list:
for field in common_keys:
row.setdefault(field, None)
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Reference: #55 (review) Confirmed: padding each file with its own column set before hashing introduced this regression. In the reported example, the previous revision incorrectly reported the unchanged The tests now cover this example, null-only column additions, non-null additions including |
Without an explicit key, a missing JSON field and an explicit null can produce different content keys even though
load_json()normalizes both rows to the same displayed data. For example,[{"id": 1}, {"id": 2, "name": "Cleo"}]and[{"id": 1, "name": null}, {"id": 2, "name": "Cleo"}]should compare without differences.Generate content keys from a temporary mapping that omits only top-level null fields. Keep the original row for display normalization, and hash nested values before converting them to strings. Non-null values, including
0,false, empty strings, lists, and objects, still contribute to row identity. Explicit-key handling is unchanged; this does not incorporate the separate duplicate-key proposal in #54.The initial revision padded each file's rows with nulls before hashing. That introduced a regression when the two files had different column sets:
[{"id": 1}, {"a": 0}]versus[{"id": 1}, {"b": 0}]incorrectly replaced the unchangedid: 1row as well as the second row. The revised approach keeps row identity independent of unrelated rows' columns and reports only the real replacement.Missing and explicit top-level null fields are equivalent for content keys. Consequently, adding a column whose value is null reports the column addition without replacing that otherwise unchanged row. The earlier description's claim that all differing-column-set behavior was unchanged was incorrect. Nested null fields are not omitted, and nested objects/lists remain distinct from JSON-looking strings.
Validation: all 40 tests pass locally on Windows with Python 3.9.13 and 3.14.4. The new regression tests failed on the previous PR head before the correction. Six subprocess CLI checks using actual JSON files match their full expected outputs, including the reported regression, the original missing/null case, a null-only column addition, and non-null additions.
git diff --checkpasses. No hosted CI result is claimed here.Related to #13.