Skip to content

fix(physical-expr): stack overflow protection#96

Merged
mhilton merged 1 commit intoupgrade-df-ver5100-afrom
mhilton/df-5100-stack-overflow-protection
Apr 22, 2026
Merged

fix(physical-expr): stack overflow protection#96
mhilton merged 1 commit intoupgrade-df-ver5100-afrom
mhilton/df-5100-stack-overflow-protection

Conversation

@mhilton
Copy link
Copy Markdown

@mhilton mhilton commented Apr 22, 2026

Add the recursive annotation to create_physical_expr to protect from a stack overflow occurring due to some pathological query expressions.

Add the recursive annotation to create_physical_expr to protect from a
stack overflow occurring due to some pathological query expressions.
@mhilton mhilton requested a review from a team April 22, 2026 10:01
/// * `e` - The logical expression
/// * `input_dfschema` - The DataFusion schema for the input, used to resolve `Column` references
/// to qualified or unqualified fields by name.
#[recursive::recursive]
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Seeing as this is only until we manage to upgrade to datafusion 52 and I don't see any situations where we wouldn't turn this on I didn't make this a feature flag.

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.

BTW this use of recursive has non trivial cost (it basically adds checks on each function call, and this function is called a lot during planning)

That being said, I suspect we won't be able to measure any diference

@mhilton mhilton merged commit e825277 into upgrade-df-ver5100-a Apr 22, 2026
60 of 63 checks passed
@alamb
Copy link
Copy Markdown
Collaborator

alamb commented Apr 22, 2026

Is this is a cherry-pick from an upstream change?

@mhilton
Copy link
Copy Markdown
Author

mhilton commented Apr 22, 2026

Is this is a cherry-pick from an upstream change?

No I chose to just add the annotation. Upstream it is a feature which I felt added more complexity to the change than I would like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants