alamb commented on code in PR #14095:
URL: https://github.com/apache/datafusion/pull/14095#discussion_r1919168076
##########
datafusion/sql/src/parser.rs:
##########
@@ -257,6 +257,9 @@ fn ensure_not_set<T>(field: &Option<T>, name: &str) ->
Result<(), ParserError> {
Ok(())
}
+// By default, allow expressions up to this deep before error
+const DEFAULT_REMAINING_DEPTH: usize = 100;
Review Comment:
I double checked that the default in sqlparser is 50 so this is 2x the size
https://docs.rs/sqlparser/latest/src/sqlparser/parser/mod.rs.html#187
I am a little worried about this as it is a hard coded limit but I think we
can always make it a config flag later
##########
datafusion/sql/src/parser.rs:
##########
@@ -257,6 +257,9 @@ fn ensure_not_set<T>(field: &Option<T>, name: &str) ->
Result<(), ParserError> {
Ok(())
}
+// By default, allow expressions up to this deep before error
Review Comment:
Maybe we can say this is 2x the default limit of sqlparser so it is clear
this is meant to *increase* the limit
```suggestion
// By default, allow expressions up to this deep before error
// (2x the default in sqlparser)
```
##########
datafusion/sqllogictest/bin/sqllogictests.rs:
##########
@@ -52,6 +52,7 @@ const SQLITE_PREFIX: &str = "sqlite";
pub fn main() -> Result<()> {
tokio::runtime::Builder::new_multi_thread()
.enable_all()
+ .thread_stack_size(4 * 1024 * 1024)
Review Comment:
I am worried about this. We have recently added the the `recursive`
protection feature precisely to avoid stack overflows (ever) but now after this
change the stacks can overflow.
Here is the stack where it overflows (precisely what the Recursion limit is
designed to prevent:
```
<sqlparser::tokenizer::Token as core::clone::Clone>::clone tokenizer.rs:52
<sqlparser::tokenizer::TokenWithSpan as core::clone::Clone>::clone
tokenizer.rs:633
core::option::Option<&T>::cloned option.rs:1917
sqlparser::parser::Parser::next_token mod.rs:3443
sqlparser::parser::Parser::parse_data_type_helper mod.rs:8097
sqlparser::parser::Parser::parse_data_type mod.rs:8083
sqlparser::parser::Parser::parse_prefix::{{closure}} mod.rs:1243
sqlparser::parser::Parser::try_parse mod.rs:3808
sqlparser::parser::Parser::maybe_parse mod.rs:3795
sqlparser::parser::Parser::parse_prefix mod.rs:1242
sqlparser::parser::Parser::parse_subexpr mod.rs:964
sqlparser::parser::Parser::parse_expr mod.rs:957
core::ops::function::FnMut::call_mut function.rs:166
sqlparser::parser::Parser::parse_comma_separated_with_trailing_commas
mod.rs:3730
sqlparser::parser::Parser::parse_comma_separated mod.rs:3715
sqlparser::parser::Parser::parse_prefix mod.rs:1381
sqlparser::parser::Parser::parse_subexpr mod.rs:964
sqlparser::parser::Parser::parse_infix mod.rs:2928
sqlparser::parser::Parser::parse_subexpr mod.rs:974
sqlparser::parser::Parser::parse_expr mod.rs:957
core::ops::function::FnMut::call_mut function.rs:166
sqlparser::parser::Parser::parse_comma_separated_with_trailing_commas
mod.rs:3730
sqlparser::parser::Parser::parse_comma_separated mod.rs:3715
sqlparser::parser::Parser::parse_prefix mod.rs:1381
sqlparser::parser::Parser::parse_subexpr mod.rs:964
sqlparser::parser::Parser::parse_infix mod.rs:2928
sqlparser::parser::Parser::parse_subexpr mod.rs:974
sqlparser::parser::Parser::parse_expr mod.rs:957
core::ops::function::FnMut::call_mut function.rs:166
sqlparser::parser::Parser::parse_comma_separated_with_trailing_commas
mod.rs:3730
sqlparser::parser::Parser::parse_comma_separated mod.rs:3715
sqlparser::parser::Parser::parse_prefix mod.rs:1381
sqlparser::parser::Parser::parse_subexpr mod.rs:964
sqlparser::parser::Parser::parse_infix mod.rs:2928
sqlparser::parser::Parser::parse_subexpr mod.rs:974
sqlparser::parser::Parser::parse_expr mod.rs:957
core::ops::function::FnMut::call_mut function.rs:166
sqlparser::parser::Parser::parse_comma_separated_with_trailing_commas
mod.rs:3730
sqlparser::parser::Parser::parse_comma_separated mod.rs:3715
sqlparser::parser::Parser::parse_prefix mod.rs:1381
sqlparser::parser::Parser::parse_subexpr mod.rs:964
sqlparser::parser::Parser::parse_infix mod.rs:2928
sqlparser::parser::Parser::parse_subexpr mod.rs:974
sqlparser::parser::Parser::parse_expr mod.rs:957
core::ops::function::FnMut::call_mut function.rs:166
sqlparser::parser::Parser::parse_comma_separated_with_trailing_commas
mod.rs:3730
sqlparser::parser::Parser::parse_comma_separated mod.rs:3715
...
std::panicking::try::do_call panicking.rs:557
__rust_try 0x000000010761f11c
[Inlined] std::panicking::try panicking.rs:520
[Inlined] std::panic::catch_unwind panic.rs:358
std::thread::Builder::spawn_unchecked_::{{closure}} mod.rs:559
core::ops::function::FnOnce::call_once{{vtable.shim}} function.rs:250
[Inlined] <alloc::boxed::Box<F,A> as
core::ops::function::FnOnce<Args>>::call_once boxed.rs:1972
[Inlined] <alloc::boxed::Box<F,A> as
core::ops::function::FnOnce<Args>>::call_once boxed.rs:1972
std::sys::pal::unix::thread::Thread::new::thread_start thread.rs:105
_pthread_start 0x00000001940832e4
```
Note @blaginin recently added the `recursive` feature to sql parser (but
it isn't yet released) which I think will allow this query to run without
having to update the stack size
- https://github.com/apache/datafusion-sqlparser-rs/pull/1522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
