[v0ropaev]

📋 Summary

Before a template is rendered, every string cell went through json.loads, so a cell with 1.10 rendered as 1.1, true as True, null as None and 1e3 as 1000.0. Following the direction in #1088, template contexts now keep strings that parse to a JSON scalar as they are and only replace strings that hold a JSON object or array, so {{ col.key }} keeps working.

🔗 Related Issue

Closes #1088

🔄 Changes

• deserialize_json_values is replaced by two helpers in processing/utils.py. prepare_template_context walks native dicts and lists and only decodes strings that hold a JSON object or array, so "1.10", "true" or "null" stay text at every level. deserialize_json_string decodes one JSON document and returns the string unchanged if it is not valid JSON.
• Template consumers use prepare_template_context. That covers the LLM completion prompt record (llm_completion.py), the image prompt (image.py), expression columns (expression.py), skip.when evaluation (skip_evaluator.py) and the schema transform processor (schema_transform.py).
• Structured and judge responses (_process_serialized_output) use deserialize_json_string, which is the same single decode as before.
• Embedding inputs (embedding.py) are taken from the target column only instead of deserializing the whole record. parse_list_string now checks that each parse attempt produced a list, so "123", "true" or "null" are embedded as written instead of failing on iteration, and quoted text like '"text"' is no longer split into characters. Items of a native list and of a serialized list are normalized the same way, ["true", "1.10"] and '["true", "1.10"]' both give ["true", "1.10"].
• Regression tests for each consumer: test_utils.py (both helpers, the no-mutation guarantee and the new parse_list_string cases), test_embedding.py (native and serialized lists, quoted text), test_llm_completion_generators.py (prompt record and one-pass structured decoding), test_expression.py, test_skip_evaluator.py, test_schema_transform.py and test_image.py.
• columns.mdx gets a short note under expression columns that text cells render as written and numbers stored as text need | int or | float before arithmetic, plus a note on text flags in skip.when.

Release note

Templates (LLM and image prompts, expression columns, skip.when and schema transforms) now see text cells as the text they contain. A cell like "1.10", "true" or "null" is no longer converted to 1.1, True or None before rendering. Arithmetic or numeric comparisons on numbers stored as text now need an explicit cast, for example {{ quantity | int + 1 }} or {{ score | float > 0.5 }}. Without a cast, such an expression fails to render and the row is dropped, and a skip.when condition that fails is treated as true, so the cell is skipped. An equality check against a number, like {{ n == 5 }}, is now simply false for the text "5". In skip.when, a text flag "false" or "null" is now a non-empty string, so when="{{ flag }}" skips the cell where it used to generate it; compare explicitly with {{ flag == 'true' }}. Columns holding a JSON object or array still support nested access like {{ result.score }}. Cells that already have a numeric or boolean dtype are not affected. Embedding columns now embed list items as written, so a native list cell ["true", "1.10"] sends "true" and "1.10" instead of "True" and "1.1", and a cell holding JSON-quoted text such as '"hello"' is embedded with its quotes.

🧪 Testing

• make test passes
• Unit tests added/updated
• E2E tests added/updated (if applicable)

All config, engine and interface tests pass (5106 passed, 11 skipped, with LC_ALL=C for the import perf test as before). The new test cases that check the new behavior fail against main's source. ruff check and ruff format --check are clean.

The repro from the issue, plus a {{ n | int + 1 }} column on "5".."8", now prints v1.10, true, True, v1e3, null and 6..9.

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• If I am an external contributor, the linked issue is open and labeled triaged
• Architecture docs updated (if applicable)

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge, with both previous findings fixed and no new actionable issues found.

Summary

This PR keeps scalar strings unchanged when preparing template inputs.

• Templates keep text cells intact while still reaching fields inside JSON values.
• Embedding columns send each target value as text without decoding scalar strings.

Review covered the full PR because the current and previously reviewed commits have different merge bases with the target commit. Tests were not run during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Record] --> B[prepare_template_context]
  B --> C[Keep scalar strings]
  B --> D[Decode JSON objects and arrays]
  C --> E[Render templates]
  D --> E
  F[Structured or judge response] --> G[deserialize_json_string]
  G --> H[Decode once]
  I[Embedding target] --> J[Read native list or parse_list_string]
  J --> K[Convert list items to text]

Loading

_{Reviews (5) · Last reviewed commit: "Merge branch 'main' into fix/template-ke..." · Reviewed by Greptile}

[andreatnvidia]

Could we also cover text flags in skip.when here? With when="{{ flag }}", text "false" and "null" now cause the cell to be skipped, and {{ not flag }} flips the other way. That follows the intended string semantics, but is easy to miss from the numeric-cast guidance. A short note recommending explicit comparisons like {{ flag == 'true' }}, plus regression cases, would help.

[v0ropaev]

Added a note right after the expression paragraph: text "false" and "null" are non-empty strings, so when="{{ flag }}" skips and {{ not flag }} doesn't, and the recommended form is {{ flag == 'true' }}. Regression cases for both forms plus the explicit comparison are in test_skip_evaluator.py (04cb220). They fail on main, as expected.

[andreatnvidia]

A few default-mode cases would help protect the compatibility boundary too. The new tests cover keep_scalar_strings=True, but the existing default-mode cases don't exercise scalar JSON strings. Consider adding "1.10", "true", "null" and a JSON-quoted string, including nested values, to pin the behavior that non-template callers still rely on.

[v0ropaev]

Done in 04cb220. Default mode now has cases for "1.10", "true", "null", a JSON-quoted string, a list of those, and a dict with them nested two levels down. They pass on both main and this branch, so they pin what non-template callers get today.

[andreatnvidia]

Thanks for keeping this focused and covering all template consumers. Please address the comments on skip-condition guidance and default-mode scalar coverage before merging.

[greptile-apps]

Required types are missing

The new test_deserialize_json_values_keep_scalar_strings function has no parameter or return annotations. The repository guide requires annotations on all functions, including tests. Add them here and to test_image_cell_generator_keeps_scalar_strings_in_prompt; the new keep_scalar_strings parameter on deserialize_json_values also needs an annotation. This requirement must be satisfied 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-engine/tests/engine/processing/test_utils.py
Line: 169

Comment:
**Required types are missing**

The new `test_deserialize_json_values_keep_scalar_strings` function has no parameter or return annotations. The repository guide requires annotations on all functions, including tests. Add them here and to `test_image_cell_generator_keeps_scalar_strings_in_prompt`; the new `keep_scalar_strings` parameter on `deserialize_json_values` also needs an annotation. This requirement must be satisfied 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!

[v0ropaev]

Fixed in 9105c1d.

[greptile-apps]

Functions break the required order

The new _loads_json_string helper appears before the public parse_list_string function. The repository guide requires public functions before private ones. Move the helper below the public functions 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-engine/src/data_designer/engine/processing/utils.py
Line: 110

Comment:
**Functions break the required order**

The new `_loads_json_string` helper appears before the public `parse_list_string` function. The repository guide requires public functions before private ones. Move the helper below the public functions 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!

[v0ropaev]

Fixed in 9105c1d.

[andreatnvidia]

Thanks for the fix and for adding the migration guidance.

I'm just not sure about the compatibility tradeoff here. Existing expressions like {{ quantity + 1 }} work with text values such as "5" today and will now need a cast. Making this opt-in would avoid that break, but having two behaviors feels a bit heavy for this. @nabinchha wdyt, is the current migration guidance enough, or do you see a better approach?

[nabinchha]

Thanks for the fix and regression coverage. I’m comfortable with the migration requirement for existing expressions and skip conditions. I’d suggest adjusting the helper design so we have consistent behavior without a keep_scalar_strings flag.

The current helper serves two purposes: preparing existing column values for templates and decoding serialized LLM responses. Could we separate those into two public functions in processing/utils.py?

Replace deserialize_json_values() with prepare_template_context() for template preparation. It would recursively process native dictionaries and lists, decoding strings only when they contain JSON objects or arrays, and always preserving scalar strings.

Add deserialize_json_string() for decoding a complete JSON document, retaining the existing fallback on invalid JSON:

def deserialize_json_string(value: str) -> Any:
    """Decode a JSON document, returning the original string if invalid."""
    try:
        return json.loads(value)
    except json.JSONDecodeError:
        return value


def prepare_template_context(data: T) -> Any:
    """Decode embedded JSON objects and arrays, preserving scalar strings."""
    if isinstance(data, str):
        loaded = deserialize_json_string(data)
        return loaded if isinstance(loaded, (dict, list)) else data

    if isinstance(data, list):
        return [prepare_template_context(item) for item in data]

    if isinstance(data, dict):
        return {
            key: prepare_template_context(value)
            for key, value in data.items()
        }

    return copy.deepcopy(data)

The callers would then use the function matching their responsibility:

• Template preparation uses prepare_template_context(record), including prompts, expressions, skip conditions, and schema transforms.
• Structured/judge output parsing uses deserialize_json_string(serialized_output). Today, those methods pass a single response string to deserialize_json_values(), which effectively performs the same JSON decoding with the same invalid-JSON fallback. Their inherited prompt preparation would use prepare_template_context().
• Embedding input preparation normalizes only the target column as text or a list of texts, rather than recursively deserializing the whole record.

The embedding path needs an accompanying fix: every parsing attempt in parse_list_string() should check that the decoded result is actually a list before processing it. Otherwise, scalar strings such as "123", "true", and "null" can reach code that attempts to iterate over a number, boolean, or None. JSON-quoted text can also become separate character inputs.

Native and serialized lists of embedding input text should behave consistently:

["true", "1.10"]       -> ["true", "1.10"]
'["true", "1.10"]'     -> ["true", "1.10"]

Currently, the native list gets scalar conversion while the serialized list preserves its string elements. The returned embedding vectors are unaffected; this concerns text normalization before the model call.

I’d cover scalar preservation in native containers, one-pass structured/judge response decoding, and consistent embedding normalization across text, native lists, and supported serialized-list formats.

Finally, this should ship with at least a minor version upgrade, with prominent release notes and migration examples. Existing arithmetic, comparisons, and skip conditions can change behavior, sometimes silently. We could also bundle this with the 1.0.0 release as part of establishing the intended type semantics.

What do you think @andreatnvidia ?

---

This review was generated by an AI assistant.

[v0ropaev]

Thanks @nabinchha, that split reads much better. Pushed in 47680e9 (also rebased on main).

deserialize_json_values is gone. Template consumers call prepare_template_context, and structured and judge responses go through deserialize_json_string. The embedding generator now reads only its target column, and parse_list_string checks for a list after every parse attempt. On main, "123" or "null" in an embedding column hit a TypeError in that function, and "'text'" came back as four single characters. One thing worth calling out is that a cell holding '"hello"' used to be embedded as hello. It now keeps the quotes, the same way templates see it. If you'd rather strip one level of JSON quoting for embeddings I can add that back.

I updated the release note in the description with the embedding change. Happy to target this at whichever release you and @andreatnvidia pick. If you keep migration notes somewhere other than the PR description, point me at it and I'll add examples there.

[nabinchha]

Thanks for addressing the design feedback, @v0ropaev.

Summary

The updated implementation separates template preparation, serialized-response decoding, and embedding-input normalization as discussed.

Findings

No outstanding actionable findings.

What Looks Good

• prepare_template_context() preserves scalar strings while decoding JSON objects and arrays for nested access.
• Structured/judge outputs use deserialize_json_string() for a single JSON decode, retaining the existing invalid-JSON fallback.
• Embedding normalization handles native and supported serialized lists consistently, with regression coverage for scalar-looking and quoted text.

Residual Risk

The 191 tests in the affected files passed, and lint and formatting passed for all 14 changed Python files. A broader engine run had 2,902 passing tests and four environment/setup failures; those four passed on a normal rerun. Validation used the PR sources over the local checkout rather than a clean checkout of the complete PR.

Please retain the migration guidance and target at least a minor release, potentially 1.0.0, as discussed. Preserving quotes in embedding text is consistent with the intended text-preservation behavior.

Verdict

Code looks ready, but please hold off on merging until we decide which release version this should ship in. The requested design changes are addressed. As discussed, this should target at least a minor release, potentially 1.0.0.

---

This review was generated by an AI assistant.

[andreatnvidia]

Thanks for splitting the helpers and updating the migration guidance.

Let’s hold off on merging until after 1.0.0. I’ll add keep-open so this won’t be closed automatically in the meantime.

Requested changes have been addressed. Merging is deferred until after 1.0.0.

[v0ropaev]

Sounds good, thanks. I'll rebase it once 1.0.0 is out.