Repository navigation
feat: preserve flexible JSON with schema-directed storage - #1062
Conversation
Preserve opted-in JSON values across Parquet artifacts, resume, seed handoffs, processors, and exports. Add preflight storage diagnostics before model calls and lifecycle regression coverage. Refs #1045 Signed-off-by: Nabin Mulepati <[email protected]>
Signed-off-by: Nabin Mulepati <[email protected]>
|
Fern preview: https://nvidia-preview-pr-1062.docs.buildwithfern.com/nemo/datadesigner
|
Address independent schema, storage, and lifecycle reviews with regressions for reference scopes, container alternatives, null and projected seed handoffs, partitioned datasets, malformed contracts, and effective storage policy. Signed-off-by: Nabin Mulepati <[email protected]>
Keep annotated DataFrame seed tables typed and contiguous through DuckDB. Match native pandas inference in preflight, normalize native nulls and strict CSV cells, and validate literal processor storage names before generation. Signed-off-by: Nabin Mulepati <[email protected]>
Signed-off-by: Nabin Mulepati <[email protected]>
Signed-off-by: Nabin Mulepati <[email protected]>
Signed-off-by: Nabin Mulepati <[email protected]>
Signed-off-by: Nabin Mulepati <[email protected]>
 [Medium impact] The PR appears safe to merge, with all previous findings addressed and no new actionable issues found. SummaryThis PR adds opt-in JSON storage for schema nodes and whole columns while keeping ordinary Python values inside DD.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| items_required = ( | ||
| certain and nonnull_types == {"array"} and node.get("minItems", 0) > len(node.get("prefixItems", [])) | ||
| ) | ||
| if "items" in node: | ||
| visit(node["items"], (*path, None), items_required, refs) |
There was a problem hiding this comment.
items_required can turn a writable array schema into a startup error. An array with prefixItems requiring an object with integer field x, trailing items: {"const": {}}, and minItems: 2 accepts [{"x": 1}, {}]. Arrow gives both elements the same struct type, so the first element supplies x and the array can be written.
The check tests the trailing {} alone and marks it as a definite failure, blocking generation. Keep this warning-only unless the check accounts for fields supplied by other array elements.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/storage/schema_preflight.py
Line: 122-126
Comment:
**Writable arrays cannot start**
`items_required` can turn a writable array schema into a startup error. An array with `prefixItems` requiring an object with integer field `x`, trailing `items: {"const": {}}`, and `minItems: 2` accepts `[{"x": 1}, {}]`. Arrow gives both elements the same struct type, so the first element supplies `x` and the array can be written.
The check tests the trailing `{}` alone and marks it as a definite failure, blocking generation. Keep this warning-only unless the check accounts for fields supplied by other array elements.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 82b17cda. Trailing-item findings are now warning-only when a nonempty prefixItems is present: prefix elements can supply struct fields that make otherwise unwritable trailing values serializable. Added regressions for the writable [{"x": 1}, {}] example and a longer array; both validate against the schema and write successfully through the actual pandas/Parquet writer. Both cases were incorrectly rejected before the fix. Updated the existing positional-array test to preserve this conservative behavior.
Validation: the two targeted preflight/context suites pass all 122 tests. The full config, engine, and interface suites pass 4,875 tests with 3 skipped. Ruff lint/format, diff checks, and commit hooks pass.
| uncertain_output = any( | ||
| processor.processor_type not in (ProcessorType.DROP_COLUMNS, ProcessorType.SCHEMA_TRANSFORM) | ||
| for processor in processors | ||
| ) or any(column.skip is not None for column in columns) |
There was a problem hiding this comment.
A skip on any column downgrades storage errors for every structured column. With an independent structured column using output_format={"const": {}}, adding a skip to an unrelated column changes its proven storage failure into a warning.
Skipping the unrelated column cannot change the empty object, so generation proceeds and fails when saving the batch. Apply the skip-based downgrade only to columns whose values can actually be skipped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/validation.py
Line: 109-112
Comment:
**Unrelated skips hide storage errors**
A `skip` on any column downgrades storage errors for every structured column. With an independent structured column using `output_format={"const": {}}`, adding a `skip` to an unrelated column changes its proven storage failure into a warning.
Skipping the unrelated column cannot change the empty object, so generation proceeds and fails when saving the batch. Apply the skip-based downgrade only to columns whose values can actually be skipped.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 82b17cda. Skip uncertainty is now limited to columns that can actually skip, either through their own gate or propagated data dependencies. The analysis follows transitive dependencies and producer side effects, respects propagate_skip=False, and leaves independent structured-column failures as errors. Added regressions for unrelated skips, direct/transitive propagation, propagation barriers, and side-effect dependencies. Unknown plugin processors retain the existing conservative downgrade.
Validation: the two targeted preflight/context suites pass all 122 tests. The full config, engine, and interface suites pass 4,875 tests with 3 skipped. Ruff lint/format, diff checks, and commit hooks pass.
|
Thanks for this, @nabinchha. The PR is large and careful, and the attention-areas section made it much easier to review. SummaryThe PR adds opt-in JSON storage for structured values. It works through three things: an FindingsWarnings — Worth addressing
Suggestions — Take it or leave itPublic contract naming — one concept, three spellings
Layering note — Parquet codec lives in
What Looks Good
Residual Risk
Structural ImpactNote: the four reported "import direction violations" are Risk: HIGH (4 core abstraction(s) modified; 4 import direction violation(s))
Import Direction Violations (4)Legal direction: interface -> engine -> config
Core Abstractions Modified
High-Connectivity Changes
Cross-Package Dependencies
VerdictNeeds changes. Please address these before merge:
The naming question in Suggestions is worth settling before the v1 contract ships. This review was generated by an AI assistant. |
| unsupported or types is None or types - {"null"} != {"object"}, | ||
| paths, | ||
| ) | ||
| elif key == "items" and isinstance(value, dict): |
There was a problem hiding this comment.
items excludes the entries covered by prefixItems, but the compiled wildcard visits every element. An integer prefix followed by an object with JSON-encoded args accepts [1, {"args": {}}], then fails during checkpoint writing when traversal reaches the integer. Rejecting this placement during config validation and asking for encoding on the enclosing array would catch it before generation.
There was a problem hiding this comment.
Fixed in 39fe3d5e. The compiler now rejects storage annotations beneath positional items during config validation, including when prefixItems and items are split across local references. Added regressions for nested-field and whole-item annotations, plus checks that encoding the enclosing array remains supported.
| # Keep encoded leaves as text through pandas' JSON conversion, | ||
| # which rounds floats. Decode them only after native fields have | ||
| # received the usual date/null normalization. | ||
| records = [json.loads(line) for line in content.splitlines()] |
There was a problem hiding this comment.
A string containing U+0085, U+2028, or U+2029 makes JSONL export fail here. Pandas leaves those characters unescaped, so splitlines() breaks a valid JSON record inside the string. Splitting only on "\n" and ignoring the trailing empty entry would preserve these values.
There was a problem hiding this comment.
Fixed in 39fe3d5e. JSONL decoding now splits only on \n and ignores the trailing empty entry. Added round-trip tests for U+0085, U+2028, and U+2029 in protected root/nested JSON and native string fields across multiple batches.
|
|
||
| def process_after_batch(self, data: pd.DataFrame, *, current_batch_number: int | None) -> pd.DataFrame: | ||
| formatted_data = self._transform(data) | ||
| missing = set(self.config.json_storage_columns) - set(formatted_data.columns) |
There was a problem hiding this comment.
When every row in a batch is dropped, _transform() returns an empty frame without columns. Even a constant template like {"payload": {"k": "v"}} then raises this error with json_storage_columns=["payload"], aborting the run before later healthy batches complete. Skip empty inputs before this check, or retain the declared output columns when constructing an empty result.
There was a problem hiding this comment.
Fixed in 39fe3d5e. Schema transforms now return immediately for inputs with zero rows, before transformation and output-column checks. Added preview/batch regressions with literal and dynamic output names that process an empty input followed by a healthy batch and verify the healthy artifact is written.
Reject positional item annotations before generation, preserve Unicode separators in JSONL records, and skip empty schema-transform batches. Add regressions for local references, nested JSON, and processor outputs. Signed-off-by: Nabin Mulepati <[email protected]>
| reference = node.get("$ref") | ||
| if reference is None or reference in references: | ||
| return False | ||
| target = resolve_local_json_pointer(root, reference) |
There was a problem hiding this comment.
One non-blocking edge case: this helper follows references inside nodes already marked for JSON storage. A referenced, annotated definition containing a named-anchor reference like #payload now fails config validation, although its contents should be opaque to the storage compiler. Returning False at annotated nodes would match _get_possible_types(). This requires an unusual custom schema, so a follow-up is fine.
andreatnvidia
left a comment
andreatnvidia
left a comment
There was a problem hiding this comment.
Thanks for fixing these, @nabinchha. The three issues I raised are addressed. The remaining custom-schema edge case is non-blocking, so I’m happy to approve.
| if node.get("maxProperties") == 0 or closed_empty: | ||
| report(path, "requires an empty object, which native Parquet cannot store as a struct", object_certain) |
There was a problem hiding this comment.
find_storage_schema_risks rejects empty objects even when an annotated child makes them writable. For example, {"type":"object","maxProperties":0,"properties":{"args":{"x-data-designer-storage":"json"}}} accepts {}. The encoder adds an explicit string field for args, so Parquet can store that value. This check still reports a definite failure and stops generation. Account for fields added by the storage plan before declaring an empty object unwritable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/storage/schema_preflight.py
Line: 110-111
Comment:
**Writable empty objects are rejected**
`find_storage_schema_risks` rejects empty objects even when an annotated child makes them writable. For example, `{"type":"object","maxProperties":0,"properties":{"args":{"x-data-designer-storage":"json"}}}` accepts `{}`. The encoder adds an explicit string field for `args`, so Parquet can store that value. This check still reports a definite failure and stops generation. Account for fields added by the storage plan before declaring an empty object unwritable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 82b17cda. Preflight now accounts for explicit physical fields supplied by JSON-encoded descendants instead of rejecting their empty logical ancestors. Added root, nested, and local-reference regressions that validate the empty sample against its schema, write it through the actual storage encoder to Parquet, and confirm validation accepts the schema. All three failed before the fix and now pass.
Validation: the two targeted preflight/context suites pass all 122 tests. The full config, engine, and interface suites pass 4,875 tests with 3 skipped. Ruff lint/format, diff checks, and commit hooks pass.
Account for physical fields supplied by encoded descendants and positional array prefixes before declaring storage failures. Limit skip uncertainty to affected columns and their dependents. Add regression coverage and reconcile the implementation plan with the delivered contract and recorded validation results. Signed-off-by: Nabin Mulepati <[email protected]>
| self.columns[name] = sorted(paths, key=_storage_path_sort_key) | ||
| return self | ||
|
|
||
| def for_columns(self, names: list[str]) -> JsonStoragePlan: |
There was a problem hiding this comment.
JsonStoragePlan.for_columns does not start with an action verb, as the repository guide requires. Rename it to restrict_to_columns or another verb-led name and update its callers. The same issue appears in _storage_type and _literal_kind. These names must meet the repository requirement before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-config/src/data_designer/config/utils/storage_schema.py
Line: 70
Comment:
**Helper names break convention**
`JsonStoragePlan.for_columns` does not start with an action verb, as the repository guide requires. Rename it to `restrict_to_columns` or another verb-led name and update its callers. The same issue appears in `_storage_type` and `_literal_kind`. These names must meet the repository requirement before merging.
**Context Used:** AGENTS.md ([source](https://github.com/nvidia-nemo/datadesigner/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 557cc5b8, following the verb-led naming convention in STYLEGUIDE.md.
- JsonStoragePlan.for_columns → restrict_to_columns
- _storage_type → _build_storage_type
- _literal_kind → _get_literal_kind
Updated all callers, including recursive calls and seed-contract tests. This is a naming-only change; storage behavior and persisted metadata are unchanged.
Validation: 467 existing tests passed across the schema compiler, codec, engine storage/preflight/seed suites, row-group buffering, and public interface JSON-storage lifecycle tests. Ruff lint/format, git diff --check, and commit hooks also pass.
Signed-off-by: Nabin Mulepati <[email protected]>
📋 Summary
Structured JSON can pass generation validation but lose dictionary-key presence or fail when Arrow infers its Parquet schema. Add explicit JSON storage for selected schema nodes or columns, preserving logical Python values through generation and DD reads. Report proven storage failures and data-dependent risks during config validation, before model calls.
🔗 Related Issue
Fixes #1045. Builds on #1046, which is already merged; this PR targets
mainand contains the storage changes.🔄 Changes
x-data-designer-storage: jsonon structured schema nodes andstorage_encoding="json"for whole columns. Strip storage annotations from model-facing schemas while retaining them in saved configs and fingerprints.json_storage_columns; projected or dropped columns cannot leak their policy into reused names.🔍 Attention Areas
🧪 Testing
39fe3d5evia pytest after addressing Andre's three inline comments.ebbbf867, before subsequent human-review fixes; that verdict is historical and is not a fresh review of39fe3d5e.create/previewpipelines and independent reproductions cover nullable scalars, precise/huge integers, mixed JSON, batch dependencies, processor artifacts, combined datasets, file/DataFrame seed handoffs, and all export formats. External model responses are stubbed where needed.git diff --checkpass. Import checks keep pandas, pyarrow, duckdb, and jsonschema unloaded until needed.[email protected] checkreports no errors; internal links pass across 90 pages. Fern's remote redirects check was skipped because authentication was unavailable.Current environment: Python 3.13.3, Pydantic 2.12.5, jsonschema 4.26.0, pandas 3.0.5, pyarrow 24.0.0, DuckDB 1.5.0.
✅ Checklist
Description updated with AI