Skip to content

fix: Add nulls in json reads if a line doesn't contain the field from the schema - #4993

Merged
colin-ho merged 5 commits into
mainfrom
colin/fix-json-read-for-missing-fileds
Sep 2, 2025
Merged

fix: Add nulls in json reads if a line doesn't contain the field from the schema#4993
colin-ho merged 5 commits into
mainfrom
colin/fix-json-read-for-missing-fileds

Conversation

@colin-ho

@colin-ho colin-ho commented Aug 18, 2025

Copy link
Copy Markdown
Collaborator

Changes Made

If the schema of an nd-json differs across records, we should fill with null instead of skipping it, otherwise it causes a series length mismatch error down the line when we create or modify the record batch.

Related Issues

Closes #4602

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

@github-actions github-actions Bot added the fix label Aug 18, 2025
@colin-ho
colin-ho marked this pull request as ready for review August 18, 2025 22:20

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR fixes a critical bug in Daft's JSON reading functionality to handle newline-delimited JSON (NDJSON) files where records have inconsistent schemas across lines. The core issue was that when a JSON record was missing a field that existed in the inferred schema, the deserializer would either throw an error or skip the field entirely, causing Arrow array length mismatches when constructing RecordBatches.

The fix implements a null-filling strategy across multiple components of the JSON reading pipeline:

  1. Deserialization Logic (decoding.rs): The fundamental change switches from iterating over record fields to iterating over schema fields. For each schema field, if the field exists in the JSON record, its value is deserialized; if missing, a null value is pushed instead. This ensures every record produces the same number of values per field.

  2. API Simplification (read.rs): The schema_is_projection parameter is removed from the deserialize_records function, simplifying the API since the new logic handles schema consistency internally. The change also updates from RecordBatch::new_unchecked to RecordBatch::new_with_size for better validation.

  3. Local Processing (local.rs): Similar null-filling logic is applied in three locations during JSON array reading, object reading within arrays, and chunked JSON parsing, replacing error-throwing behavior with null insertion.

  4. Test Coverage (test_json_roundtrip.py): Comprehensive tests are added to verify the null-filling behavior works correctly for both single-file and multi-file scenarios, including support for gzip compression.

This change aligns Daft's JSON reader with standard JSON processing behavior where missing fields are treated as null values, enabling proper handling of real-world JSON data that often has heterogeneous schemas. The fix also enables support for compressed JSON files (.json.gz) as mentioned in the related issue.

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it fixes a clear bug and follows standard JSON processing patterns
  • Score reflects well-structured changes with comprehensive test coverage, though the complexity of multi-file JSON handling adds some risk
  • Pay close attention to the core deserialization logic in src/daft-json/src/decoding.rs to ensure the null-filling behavior works correctly across all JSON formats

4 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@codecov

codecov Bot commented Aug 19, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.19%. Comparing base (ae638e5) to head (7571abf).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-json/src/local.rs 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4993      +/-   ##
==========================================
- Coverage   75.27%   75.19%   -0.09%     
==========================================
  Files         949      949              
  Lines      132520   132523       +3     
==========================================
- Hits        99761    99647     -114     
- Misses      32759    32876     +117     
Files with missing lines Coverage Δ
src/daft-json/src/decoding.rs 73.41% <100.00%> (+6.74%) ⬆️
src/daft-json/src/read.rs 85.04% <100.00%> (-9.24%) ⬇️
src/daft-json/src/local.rs 83.05% <50.00%> (+0.70%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@colin-ho

Copy link
Copy Markdown
Collaborator Author

@universalmind303 could you give this a look?

@colin-ho
colin-ho requested review from desmondcheongzx and removed request for universalmind303 September 2, 2025 16:11

@desmondcheongzx desmondcheongzx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!



@pytest.mark.parametrize("compression", [None, "gzip"])
def test_roundtrip_ndjson_with_mismatched_schema_between_files(tmp_path, compression):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other interesting case to test for is if path2 is read before path1. In this case we drop the city column (since it wasn't present during schema inference), which seems good enough to me.

@colin-ho
colin-ho enabled auto-merge (squash) September 2, 2025 22:13
@colin-ho
colin-ho merged commit db18b88 into main Sep 2, 2025
53 of 54 checks passed
@colin-ho
colin-ho deleted the colin/fix-json-read-for-missing-fileds branch September 2, 2025 23:49
venkateshdb pushed a commit to venkateshdb/Daft that referenced this pull request Sep 6, 2025
… the schema (Eventual-Inc#4993)

## Changes Made

If the schema of an nd-json differs across records, we should fill with
null instead of skipping it, otherwise it causes a series length
mismatch error down the line when we create or modify the record batch.

## Related Issues

Closes Eventual-Inc#4602

## Checklist

- [ ] Documented in API Docs (if applicable)
- [ ] Documented in User Guide (if applicable)
- [ ] If adding a new documentation page, doc is added to
`docs/mkdocs.yml` navigation
- [ ] Documentation builds and is formatted properly (tag @/ccmao1130
for docs review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support compression for read_json

2 participants