Skip to content

Handling circular references#13

Merged
giograno merged 8 commits into
mainfrom
circular-reference
Apr 23, 2026
Merged

Handling circular references#13
giograno merged 8 commits into
mainfrom
circular-reference

Conversation

@giograno
Copy link
Copy Markdown
Member

@giograno giograno commented Dec 11, 2025

We did not currently handle circular references in Python types.
We had a few in the Python codebase, e.g., Expression in ce or Configuration in emr.

To solve these issues, we are going to use ForwardRefs to stop the cycle and avoid infinite recursions.
In a nutshell, when processing the record schemas, we keep track of all the visited names and emit ForwardRefs when we see a name twice.

I also added a bunch of tests to verify this behavior, which is especially tricky when using the wrapped types and the namespaces.

@giograno giograno requested review from bentsku and purcell December 11, 2025 08:58
@giograno giograno changed the title Handling circular dependencies Handling circular references Dec 11, 2025
@giograno giograno marked this pull request as ready for review February 6, 2026 14:13
@giograno giograno marked this pull request as draft February 6, 2026 14:25
@giograno giograno force-pushed the circular-reference branch from 98893d0 to 35ce48d Compare April 22, 2026 16:13
@giograno giograno marked this pull request as ready for review April 22, 2026 17:04
@giograno giograno marked this pull request as draft April 23, 2026 05:58
@giograno giograno force-pushed the circular-reference branch from 64480d6 to 1cf9a3a Compare April 23, 2026 13:50
@giograno giograno marked this pull request as ready for review April 23, 2026 13:58
Copy link
Copy Markdown

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM, looks like a safe change! Does it require any changes in custom schemas when updating the library?

"fields": [
{"name": REF_ID_KEY, "type": ["null", "long"], "default": None},
{"name": REF_DATA_KEY, "type": inner_schema},
{"name": REF_DATA_KEY, "type": build_inner(names)},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

neat 👌

@giograno
Copy link
Copy Markdown
Member Author

LGTM, looks like a safe change! Does it require any changes in custom schemas when updating the library?

Yep, we need to add the processing parameter (or **kwargs) to the BaseRecord and the adapter. I did try it already locally.

@giograno giograno merged commit 7665caa into main Apr 23, 2026
6 checks passed
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.

2 participants