Annotate `Expr::get_type()` with recursive

peter-toth commented 1 week ago

Which issue does this PR close?

Follow-up to https://github.com/apache/datafusion/pull/13310, closes https://github.com/apache/datafusion/issues/9375.

Rationale for this change

This is a small follow-up change to the previous PR to fix the stack overflow issue in https://github.com/apache/datafusion/issues/9375.

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

There was a stack overflow with debug build due to sqlparser, and this didn't come up with release build: https://github.com/apache/datafusion/issues/9375#issuecomment-2469909998.
And there was a different stack overflow with release build: https://github.com/apache/datafusion/issues/9375#issuecomment-2470000115.

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.

peter-toth commented 1 week ago

cc @gruuya, @alamb

jayzhan211 commented 1 week ago

Thanks @peter-toth @gruuya

findepi commented 5 days ago

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

apache / datafusion