kosiew commented on code in PR #22943:
URL: https://github.com/apache/datafusion/pull/22943#discussion_r3426542847
##########
datafusion/sql/src/planner.rs:
##########
@@ -276,6 +276,13 @@ pub struct PlannerContext {
set_expr_left_schema: Option<DFSchemaRef>,
/// The parameters of all lambdas seen so far
lambda_parameters: HashMap<String, FieldRef>,
+ /// Depth of the SQL expression currently being planned, relative to the
Review Comment:
Nice catch carrying `expr_depth` across cloned query and subquery contexts.
The `# Cloning` docs above still mention that only `ctes` are truly cloned.
Could we update that comment to reflect the current behavior as well? That
should help avoid future changes accidentally resetting or special-casing
`expr_depth` during context cloning.
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -1494,6 +1552,71 @@ mod tests {
test_stack_overflow!(test_stack_overflow_2048, 2048);
test_stack_overflow!(test_stack_overflow_4096, 4096);
test_stack_overflow!(test_stack_overflow_8192, 8192);
+
+ /// Plan `sql` as an expression against an empty schema using the default
+ /// configuration (so the default expression-depth limit applies).
+ fn plan_expr_with_default_limit(sql: &str) -> Result<Expr> {
+ let schema = DFSchema::empty();
+ let mut planner_context = PlannerContext::default();
+
+ let dialect = GenericDialect {};
+ let mut parser = Parser::new(&dialect).try_with_sql(sql).unwrap();
+ let sql_expr = parser.parse_expr().unwrap();
+
+ let context_provider = TestContextProvider::new();
+ let sql_to_rel = SqlToRel::new(&context_provider);
+ sql_to_rel.sql_expr_to_logical_expr(sql_expr, &schema, &mut
planner_context)
+ }
+
+ // The default expression-depth limit is `recursion_limit * 10`.
+ const DEFAULT_EXPR_DEPTH_LIMIT: usize = 50 * EXPR_DEPTH_LIMIT_MULTIPLIER;
+
+ #[test]
+ fn test_expr_depth_limit_within_limit() {
+ // A flat chain of N predicates produces a tree of depth N + 1, so the
+ // deepest chain accepted by the default limit has
+ // `DEFAULT_EXPR_DEPTH_LIMIT - 1` predicates.
+ plan_or_chain(DEFAULT_EXPR_DEPTH_LIMIT - 1).unwrap();
+ }
+
+ #[test]
+ fn test_expr_depth_limit_exceeded_returns_error() {
Review Comment:
The new regression tests cover `sql_expr_to_logical_expr` directly, which is
great. As a follow-up, it might be worth adding a small end-to-end SQL planning
test too. That would help demonstrate that the public
`SessionContext`/`SessionState` path surfaces this planning error before
validation or optimizer cloning has a chance to recurse into the deeply nested
`Expr`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
