Skip to content

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