Skip to content

Add convert.parse object parsing wrangle - #1187

Merged
mborodii-prog merged 4 commits into
mainfrom
align-convert-to-froms
Sep 22, 2026
Merged

mborodii-prog merged 4 commits into
mainfrom
align-convert-to-froms

Conversation

@ebhills

@ebhills ebhills commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Adds convert.parse for cells containing JSON, Python literals, YAML-like structures, or accidentally quoted objects. It produces JSON-compatible values with explicit expected-type and fallback handling while preserving the behavior of the four existing JSON/YAML conversion wrangles.

Linked issue

Closes #1189

What changes

  • Parses JSON first, then Python literals, then restricted YAML, with one additional parse for an accidentally quoted structure.
  • Supports expected: any, dictionary, list, or scalar, including per-column categories and defaults. Missing cells remain empty when no default is supplied.
  • Preserves parsed values in an object-typed output column aligned with the source index, so large integers remain exact alongside floats or missing values.
  • Normalizes NumPy values in both parsed objects and defaults. Invalid explicit defaults raise ValueError; mutable defaults are copied independently for each row.
  • Handles recursion-limit failures as conversion failures: excessively nested input uses the configured default or raises a contextual ValueError. Deeply nested defaults are rejected with a contextual ValueError, and error reporting remains safe for deeply nested materialized objects.
  • Rejects YAML anchors, aliases, and explicit tags before loading, preventing alias expansion during normalization.
  • Uses JSON-style YAML numeric resolution: identifiers such as 00123, 12:34, and 0xFF remain strings, while ordinary decimal and scientific-notation numbers remain numeric.
  • Defines scalar and array forms of expected separately in the generated recipe schema.

Scope is limited to wrangles/recipe_wrangles/convert.py and tests/recipes/wrangles/test_convert.py.

How it was verified

Validated commit f30e6be1, which includes current main (0df6c569).

  • 181 focused tests passed locally on Python 3.13: tests/recipes/wrangles/test_convert.py, tests/recipes/wrangles/test_main.py::TestWrangleSchema, and tests/test_dataframe.py. Credentials were removed from the test process and network access was blocked.
  • Regression coverage includes both PyYAML loaders, quoted alias graphs, numeric identifiers, independent normalized defaults, rejected unsupported defaults, and valid/invalid scalar and array expected values.
  • The 17 new regression cases failed against the previous implementation and pass with these fixes. They cover large integers beside floats or missing values, non-default row indexes, exact values rendered through a recipe, and excessive nesting in JSON, YAML, and materialized objects through both the DataFrame API and recipe runner, with and without defaults.
  • Full recipe-schema generation and validation passed offline using the bundled Draft 7 meta-schema; complete recipes with invalid expected values were rejected.
  • git diff --check passed. Implementation comparison confirmed that from_json, from_yaml, to_json, and to_yaml remain unchanged.
  • Fresh GitHub CI run is in progress for published head f30e6be1. Local checks do not establish completed CI or deployment.

Compatibility and risk

This is an additive wrangle. Existing converters and their callers retain their current behavior. No credentials or external services are required by convert.parse, and no deployment is included.

The new parser deliberately rejects YAML anchors, aliases, explicit tags, and non-JSON-compatible values/defaults. YAML-only numeric forms are retained as text. Its output columns retain object dtype to preserve Python value types and integer precision; excessive nesting follows the same default/error contract as other conversion failures. These rules are documented in its schema docstring and covered by regression tests.

Future direction: convert.parse could eventually replace some uses of from_yaml or from_json; migration or replacement of those wrangles remains outside this PR, as does recursively parsing serialized strings nested inside objects.

Rollback: remove any newly introduced convert.parse recipe usage, then revert this PR. Existing converters require no migration.

The PR is Ready with changes requested. The two current review findings are fixed in f30e6be1 and await reviewer verification; the earlier three conversations are resolved. After fresh CI passes and both current threads have fixing-commit replies, the human assignee should re-request review from mborodii-prog. The reviewer should verify the fixes, resolve the two conversations, and submit a fresh approval.

Ready-for-review checklist

  • One human delivery owner is assigned
  • The linked issue and intended milestone are correct
  • The branch is current with main and has no merge conflicts
  • Focused tests pass
  • New or changed behavior has direct test coverage
  • Documentation/schema/configuration is updated where applicable
  • The PR contains no unrelated changes
  • The PR description reflects the branch's current scope and latest validation
  • One primary reviewer is requested only when this PR is ready

No release milestone is selected.

See the pull request workflow.

 - Introduce a new `convert.parse` recipe wrangle that parses JSON, Python literals, and YAML-like object text into JSON-compatible Python values. The implementation adds expected-type validation, per-column defaults, missing-value handling, normalization for numpy-backed objects, and safer YAML scalar resolution.

 - Eventually could replace from_yaml, from_json

-  Tests cover valid inputs, quoted structures, defaults, type mismatches, invalid values, and multi-column behavior.

Copilot AI 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.

🟡 Changes recommended

Default normalization, schema validation, and YAML alias resource-safety issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds convert.parse for converting JSON, Python literals, and YAML-like text into JSON-compatible Python values.

Changes:

  • Adds parsing, normalization, expected-type validation, and fallback handling.
  • Adds comprehensive recipe-level tests for supported inputs and edge cases.
File summaries
File Description
wrangles/recipe_wrangles/convert.py Implements convert.parse and its schema.
tests/recipes/wrangles/test_convert.py Tests parsing, defaults, validation, and multiple columns.
Review details

Suppressed comments (1)

wrangles/recipe_wrangles/convert.py:645

  • The invalid/type-mismatch fallback also skips _normalize_json_compatible, so NumPy-backed or otherwise unsupported defaults can escape unchanged. Apply the same normalization as successful parsed values so every return path honors the JSON-compatible result contract.
                    return _copy.deepcopy(col_default)
  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread wrangles/recipe_wrangles/convert.py
Comment thread wrangles/recipe_wrangles/convert.py Outdated
Comment thread wrangles/recipe_wrangles/convert.py
@ebhills
ebhills marked this pull request as draft September 20, 2026 23:37
@ebhills ebhills self-assigned this Sep 20, 2026
@ebhills
ebhills marked this pull request as ready for review September 20, 2026 23:48
_convert_value(value)
for value in df[input_column]
]

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.

@ebhills
Assigning the parsed results as a plain list lets pandas infer a float dtype when the column contains both integers and floats. This silently corrupts large integers: parsing ["9007199254740993", "1.5"] produces [9007199254740992.0, 1.5].
Reproduced through a recipe and confirmed by rendering value={{ parsed }} as text before exporting to Excel, so this is not an Excel display issue.
Please assign the results using an explicitly object-typed Series with index=df.index, and add regression tests for a large integer alongside a float or a missing value.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in f30e6be1. convert.parse now assigns an explicitly object-typed Series with index=df.index, preserving the exact Python integer 9007199254740993 alongside either a float or a missing value.

Regression coverage: test_large_integer_precision_with_mixed_scalars checks exact values, Python types, and a non-default row index. test_large_integer_precision_survives_recipe_rendering verifies the recipe path and exact value={{ parsed }} text for both mixed-column cases. These four cases failed before the fix and now pass; all 181 focused conversion/schema/DataFrame tests passed locally. Fresh CI is running.

Recommended disposition: Comment only

Next steps

  1. PR assignee: After CI passes, re-request review from mborodii-prog on Add convert.parse object parsing wrangle #1187.
  2. Reviewer: Verify the precision and index regressions, resolve this conversation, and submit a fresh approval once both fixes are verified.

Comment thread wrangles/recipe_wrangles/convert.py Outdated
f"Result is not a valid {col_expected}"
)
return result
except (TypeError, ValueError) as error:

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.

@ebhills Deeply nested input raises RecursionError, which this handler does not catch. For example, parsing "[" * 1100 + "0" + "]" * 1100 with default={} aborts the recipe instead of returning the configured fallback.
Please handle recursion-limit failures as conversion failures, or enforce a depth limit that raises a handled exception. Add regression coverage confirming that excessive nesting returns the configured default, while the same input without a default raises a contextual ValueError.

read:

  • test:
    rows: 1
    values:
    original: placeholder

wrangles:

  • create.jinja:
    output: original
    template:
    string: '{{ "[" * 1100 }}0{{ "]" * 1100 }}'
  • convert.parse:
    input: original
    output: parsed
    default: {}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in f30e6be1. The conversion handler now catches RecursionError: excessively nested input returns an independent copy of the configured default, or raises the contextual ValueError when no default is supplied. Error formatting also handles deeply nested materialized objects safely, and an excessively nested explicit default raises a contextual invalid-default error.

Regression coverage: test_excessive_nesting_uses_default and test_excessive_nesting_without_default_has_context cover JSON, YAML, and materialized objects through both the DataFrame API and recipe runner. test_excessively_nested_default_is_rejected covers default validation. These 13 cases failed before the fix and now pass; all 181 focused conversion/schema/DataFrame tests passed locally. Fresh CI is running.

Recommended disposition: Comment only

Next steps

  1. PR assignee: After CI passes, re-request review from mborodii-prog on Add convert.parse object parsing wrangle #1187.
  2. Reviewer: Verify fallback and contextual-error behavior for excessive nesting, resolve this conversation, and submit a fresh approval once both fixes are verified.

@mborodii-prog
mborodii-prog self-requested a review September 22, 2026 07:59
@mborodii-prog
mborodii-prog merged commit 9550473 into main Sep 22, 2026
32 of 39 checks passed
@mborodii-prog
mborodii-prog deleted the align-convert-to-froms branch September 22, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tolerant convert.parse for JSON-like cell values

3 participants