Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate Expr::get_type() with recursive #13376

Merged

Conversation

peter-toth
Copy link
Contributor

Which issue does this PR close?

Follow-up to #13310, closes #9375.

Rationale for this change

This is a small follow-up change to the previous PR to fix the stack overflow issue in #9375.

Please note that during the investigation 2 different issues were discovered:

This PR fixes only the 2nd issue.
Most likely the 1st issue can also be fixed with stacker/recursive, but the change would be required in https://github.com/apache/datafusion-sqlparser-rs.

What changes are included in this PR?

This PR annotates Expr::get_type with #[recursive]

Are these changes tested?

Yes, with the blowout2.zip test provided in the issue description:

% cargo run --release -- -f ~/Downloads/blowout2.sql

...
DataFusion CLI v43.0.0
0 row(s) fetched.
Elapsed 0.007 seconds.

+-------+
| count |
+-------+
| 1     |
+-------+
1 row(s) fetched.
Elapsed 0.007 seconds.

+---+
| x |
+---+
| 1 |
+---+
1 row(s) fetched.
Elapsed 1.262 seconds.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Nov 12, 2024
@peter-toth peter-toth changed the title annotate get_type with recursive Annotate Expr::get_type() with recursive Nov 12, 2024
@peter-toth
Copy link
Contributor Author

cc @gruuya, @alamb

Copy link
Contributor

@gruuya gruuya left a comment

Choose a reason for hiding this comment

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

I tested this locally too, and can verify that it works, thanks @peter-toth!

Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

@jayzhan211
Copy link
Contributor

Thanks @peter-toth @gruuya

@jayzhan211 jayzhan211 merged commit 2a2de82 into apache:main Nov 13, 2024
26 checks passed
alamb pushed a commit to alamb/datafusion that referenced this pull request Nov 13, 2024
@findepi
Copy link
Member

findepi commented Nov 16, 2024

Would we benefit from any Large OR list test to prevent regressions, especially as we write new code or new optimizers?
(eg expression simplification)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large OR list overflows the stack
4 participants