Skip to content

Commit e99e02b

Browse files
alambalamb
authored
Fix recursive-protection feature flag (#13887)
* Fix recursive-protection feature flag * rename feature flag to be consistent * Make default * taplo format
1 parent 901a094 commit e99e02b

File tree

22 files changed

+108
-36
lines changed

22 files changed

+108
-36
lines changed

‎README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ Default features:
113113
- `regex_expressions`: regular expression functions, such as `regexp_match`
114114
- `unicode_expressions`: Include unicode aware functions such as `character_length`
115115
- `unparser`: enables support to reverse LogicalPlans back into SQL
116-
- `recursive-protection`: uses [recursive](https://docs.rs/recursive/latest/recursive/) for stack overflow protection.
116+
- `recursive_protection`: uses [recursive](https://docs.rs/recursive/latest/recursive/) for stack overflow protection.
117117

118118
Optional features:
119119

‎datafusion-cli/Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎datafusion-cli/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ datafusion = { path = "../datafusion/core", version = "43.0.0", features = [
4545
"datetime_expressions",
4646
"encoding_expressions",
4747
"parquet",
48+
"recursive_protection",
4849
"regex_expressions",
4950
"unicode_expressions",
5051
"compression",

‎datafusion/common/Cargo.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,11 @@ name = "datafusion_common"
3636
path = "src/lib.rs"
3737

3838
[features]
39-
default = ["recursive-protection"]
4039
avro = ["apache-avro"]
4140
backtrace = []
4241
pyarrow = ["pyo3", "arrow/pyarrow", "parquet"]
4342
force_hash_collisions = []
44-
recursive-protection = ["dep:recursive"]
43+
recursive_protection = ["dep:recursive"]
4544

4645
[dependencies]
4746
ahash = { workspace = true }

‎datafusion/common/src/tree_node.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ pub trait TreeNode: Sized {
124124
/// TreeNodeVisitor::f_up(ChildNode2)
125125
/// TreeNodeVisitor::f_up(ParentNode)
126126
/// ```
127-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
127+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
128128
fn visit<'n, V: TreeNodeVisitor<'n, Node = Self>>(
129129
&'n self,
130130
visitor: &mut V,
@@ -174,7 +174,7 @@ pub trait TreeNode: Sized {
174174
/// TreeNodeRewriter::f_up(ChildNode2)
175175
/// TreeNodeRewriter::f_up(ParentNode)
176176
/// ```
177-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
177+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
178178
fn rewrite<R: TreeNodeRewriter<Node = Self>>(
179179
self,
180180
rewriter: &mut R,
@@ -197,7 +197,7 @@ pub trait TreeNode: Sized {
197197
&'n self,
198198
mut f: F,
199199
) -> Result<TreeNodeRecursion> {
200-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
200+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
201201
fn apply_impl<'n, N: TreeNode, F: FnMut(&'n N) -> Result<TreeNodeRecursion>>(
202202
node: &'n N,
203203
f: &mut F,
@@ -232,7 +232,7 @@ pub trait TreeNode: Sized {
232232
self,
233233
mut f: F,
234234
) -> Result<Transformed<Self>> {
235-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
235+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
236236
fn transform_down_impl<N: TreeNode, F: FnMut(N) -> Result<Transformed<N>>>(
237237
node: N,
238238
f: &mut F,
@@ -256,7 +256,7 @@ pub trait TreeNode: Sized {
256256
self,
257257
mut f: F,
258258
) -> Result<Transformed<Self>> {
259-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
259+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
260260
fn transform_up_impl<N: TreeNode, F: FnMut(N) -> Result<Transformed<N>>>(
261261
node: N,
262262
f: &mut F,
@@ -371,7 +371,7 @@ pub trait TreeNode: Sized {
371371
mut f_down: FD,
372372
mut f_up: FU,
373373
) -> Result<Transformed<Self>> {
374-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
374+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
375375
fn transform_down_up_impl<
376376
N: TreeNode,
377377
FD: FnMut(N) -> Result<Transformed<N>>,
@@ -2349,7 +2349,7 @@ pub(crate) mod tests {
23492349
Ok(())
23502350
}
23512351

2352-
#[cfg(feature = "recursive-protection")]
2352+
#[cfg(feature = "recursive_protection")]
23532353
#[test]
23542354
fn test_large_tree() {
23552355
let mut item = TestTreeNode::new_leaf("initial".to_string());

‎datafusion/core/Cargo.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ default = [
5959
"unicode_expressions",
6060
"compression",
6161
"parquet",
62+
"recursive_protection",
6263
]
6364
encoding_expressions = ["datafusion-functions/encoding_expressions"]
6465
# Used for testing ONLY: causes all values to hash to the same value (test for collisions)
@@ -69,6 +70,13 @@ pyarrow = ["datafusion-common/pyarrow", "parquet"]
6970
regex_expressions = [
7071
"datafusion-functions/regex_expressions",
7172
]
73+
recursive_protection = [
74+
"datafusion-common/recursive_protection",
75+
"datafusion-expr/recursive_protection",
76+
"datafusion-optimizer/recursive_protection",
77+
"datafusion-physical-optimizer/recursive_protection",
78+
"datafusion-sql/recursive_protection",
79+
]
7280
serde = ["arrow-schema/serde"]
7381
string_expressions = ["datafusion-functions/string_expressions"]
7482
unicode_expressions = [

‎datafusion/expr/Cargo.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@ name = "datafusion_expr"
3636
path = "src/lib.rs"
3737

3838
[features]
39-
default = ["recursive-protection"]
40-
recursive-protection = ["dep:recursive"]
39+
recursive_protection = ["dep:recursive"]
4140

4241
[dependencies]
4342
arrow = { workspace = true }

‎datafusion/expr/src/expr_schema.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ impl ExprSchemable for Expr {
9999
/// expression refers to a column that does not exist in the
100100
/// schema, or when the expression is incorrectly typed
101101
/// (e.g. `[utf8] + [bool]`).
102-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
102+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
103103
fn get_type(&self, schema: &dyn ExprSchema) -> Result<DataType> {
104104
match self {
105105
Expr::Alias(Alias { expr, name, .. }) => match &**expr {

‎datafusion/expr/src/logical_plan/tree_node.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,7 @@ impl LogicalPlan {
668668

669669
/// Visits a plan similarly to [`Self::visit`], including subqueries that
670670
/// may appear in expressions such as `IN (SELECT ...)`.
671-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
671+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
672672
pub fn visit_with_subqueries<V: for<'n> TreeNodeVisitor<'n, Node = Self>>(
673673
&self,
674674
visitor: &mut V,
@@ -687,7 +687,7 @@ impl LogicalPlan {
687687
/// Similarly to [`Self::rewrite`], rewrites this node and its inputs using `f`,
688688
/// including subqueries that may appear in expressions such as `IN (SELECT
689689
/// ...)`.
690-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
690+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
691691
pub fn rewrite_with_subqueries<R: TreeNodeRewriter<Node = Self>>(
692692
self,
693693
rewriter: &mut R,
@@ -706,7 +706,7 @@ impl LogicalPlan {
706706
&self,
707707
mut f: F,
708708
) -> Result<TreeNodeRecursion> {
709-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
709+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
710710
fn apply_with_subqueries_impl<
711711
F: FnMut(&LogicalPlan) -> Result<TreeNodeRecursion>,
712712
>(
@@ -741,7 +741,7 @@ impl LogicalPlan {
741741
self,
742742
mut f: F,
743743
) -> Result<Transformed<Self>> {
744-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
744+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
745745
fn transform_down_with_subqueries_impl<
746746
F: FnMut(LogicalPlan) -> Result<Transformed<LogicalPlan>>,
747747
>(
@@ -766,7 +766,7 @@ impl LogicalPlan {
766766
self,
767767
mut f: F,
768768
) -> Result<Transformed<Self>> {
769-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
769+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
770770
fn transform_up_with_subqueries_impl<
771771
F: FnMut(LogicalPlan) -> Result<Transformed<LogicalPlan>>,
772772
>(
@@ -794,7 +794,7 @@ impl LogicalPlan {
794794
mut f_down: FD,
795795
mut f_up: FU,
796796
) -> Result<Transformed<Self>> {
797-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
797+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
798798
fn transform_down_up_with_subqueries_impl<
799799
FD: FnMut(LogicalPlan) -> Result<Transformed<LogicalPlan>>,
800800
FU: FnMut(LogicalPlan) -> Result<Transformed<LogicalPlan>>,

‎datafusion/optimizer/Cargo.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@ name = "datafusion_optimizer"
3636
path = "src/lib.rs"
3737

3838
[features]
39-
default = ["recursive-protection"]
40-
recursive-protection = ["dep:recursive"]
39+
recursive_protection = ["dep:recursive"]
4140

4241
[dependencies]
4342
arrow = { workspace = true }

‎datafusion/optimizer/src/analyzer/subquery.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ fn check_correlations_in_subquery(inner_plan: &LogicalPlan) -> Result<()> {
128128
}
129129

130130
// Recursively check the unsupported outer references in the sub query plan.
131-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
131+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
132132
fn check_inner_plan(inner_plan: &LogicalPlan, can_contain_outer_ref: bool) -> Result<()> {
133133
if !can_contain_outer_ref && inner_plan.contains_outer_reference() {
134134
return plan_err!("Accessing outer reference columns is not allowed in the plan");

‎datafusion/optimizer/src/common_subexpr_eliminate.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ impl OptimizerRule for CommonSubexprEliminate {
531531
None
532532
}
533533

534-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
534+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
535535
fn rewrite(
536536
&self,
537537
plan: LogicalPlan,

‎datafusion/optimizer/src/eliminate_cross_join.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ impl OptimizerRule for EliminateCrossJoin {
7979
true
8080
}
8181

82-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
82+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
8383
fn rewrite(
8484
&self,
8585
plan: LogicalPlan,

‎datafusion/optimizer/src/optimize_projections/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ impl OptimizerRule for OptimizeProjections {
109109
/// columns.
110110
/// - `Ok(None)`: Signal that the given logical plan did not require any change.
111111
/// - `Err(error)`: An error occurred during the optimization process.
112-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
112+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
113113
fn optimize_projections(
114114
plan: LogicalPlan,
115115
config: &dyn OptimizerConfig,

‎datafusion/physical-optimizer/Cargo.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,7 @@ rust-version = { workspace = true }
3232
workspace = true
3333

3434
[features]
35-
default = ["recursive-protection"]
36-
recursive-protection = ["dep:recursive"]
35+
recursive_protection = ["dep:recursive"]
3736

3837
[dependencies]
3938
arrow = { workspace = true }

‎datafusion/physical-optimizer/src/aggregate_statistics.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ impl AggregateStatistics {
4141
}
4242

4343
impl PhysicalOptimizerRule for AggregateStatistics {
44-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
44+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
4545
fn optimize(
4646
&self,
4747
plan: Arc<dyn ExecutionPlan>,

‎datafusion/sql/Cargo.toml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,10 @@ name = "datafusion_sql"
3636
path = "src/lib.rs"
3737

3838
[features]
39-
default = ["unicode_expressions", "unparser", "recursive-protection"]
39+
default = ["unicode_expressions", "unparser"]
4040
unicode_expressions = []
4141
unparser = []
42-
recursive-protection = ["dep:recursive"]
42+
recursive_protection = ["dep:recursive"]
4343

4444
[dependencies]
4545
arrow = { workspace = true }

‎datafusion/sql/src/expr/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
196196

197197
/// Internal implementation. Use
198198
/// [`Self::sql_expr_to_logical_expr`] to plan exprs.
199-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
199+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
200200
fn sql_expr_to_logical_expr_internal(
201201
&self,
202202
sql: SQLExpr,

‎datafusion/sql/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ mod query;
4343
mod relation;
4444
mod select;
4545
mod set_expr;
46+
mod stack;
4647
mod statement;
4748
#[cfg(feature = "unparser")]
4849
pub mod unparser;

‎datafusion/sql/src/query.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use std::sync::Arc;
1919

2020
use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
2121

22+
use crate::stack::StackGuard;
2223
use datafusion_common::{not_impl_err, Constraints, DFSchema, Result};
2324
use datafusion_expr::expr::Sort;
2425
use datafusion_expr::{
@@ -62,10 +63,11 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
6263
// The functions called from `set_expr_to_plan()` need more than 128KB
6364
// stack in debug builds as investigated in:
6465
// https://github.com/apache/datafusion/pull/13310#discussion_r1836813902
65-
let min_stack_size = recursive::get_minimum_stack_size();
66-
recursive::set_minimum_stack_size(256 * 1024);
67-
let plan = self.set_expr_to_plan(other, planner_context)?;
68-
recursive::set_minimum_stack_size(min_stack_size);
66+
let plan = {
67+
// scope for dropping _guard
68+
let _guard = StackGuard::new(256 * 1024);
69+
self.set_expr_to_plan(other, planner_context)
70+
}?;
6971
let oby_exprs = to_order_by_exprs(query.order_by)?;
7072
let order_by_rex = self.order_by_to_sort_expr(
7173
oby_exprs,

‎datafusion/sql/src/set_expr.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use datafusion_expr::{LogicalPlan, LogicalPlanBuilder};
2121
use sqlparser::ast::{SetExpr, SetOperator, SetQuantifier};
2222

2323
impl<S: ContextProvider> SqlToRel<'_, S> {
24-
#[cfg_attr(feature = "recursive-protection", recursive::recursive)]
24+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
2525
pub(super) fn set_expr_to_plan(
2626
&self,
2727
set_expr: SetExpr,

‎datafusion/sql/src/stack.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
pub use inner::StackGuard;
19+
20+
/// A guard that sets the minimum stack size for the current thread to `min_stack_size` bytes.
21+
#[cfg(feature = "recursive_protection")]
22+
mod inner {
23+
/// Sets the stack size to `min_stack_size` bytes on call to `new()` and
24+
/// resets to the previous value when this structure is dropped.
25+
pub struct StackGuard {
26+
previous_stack_size: usize,
27+
}
28+
29+
impl StackGuard {
30+
/// Sets the stack size to `min_stack_size` bytes on call to `new()` and
31+
/// resets to the previous value when this structure is dropped.
32+
pub fn new(min_stack_size: usize) -> Self {
33+
let previous_stack_size = recursive::get_minimum_stack_size();
34+
recursive::set_minimum_stack_size(min_stack_size);
35+
Self {
36+
previous_stack_size,
37+
}
38+
}
39+
}
40+
41+
impl Drop for StackGuard {
42+
fn drop(&mut self) {
43+
recursive::set_minimum_stack_size(self.previous_stack_size);
44+
}
45+
}
46+
}
47+
48+
/// A stub implementation of the stack guard when the recursive protection
49+
/// feature is not enabled
50+
#[cfg(not(feature = "recursive_protection"))]
51+
mod inner {
52+
/// A stub implementation of the stack guard when the recursive protection
53+
/// feature is not enabled that does nothing
54+
pub struct StackGuard;
55+
56+
impl StackGuard {
57+
/// A stub implementation of the stack guard when the recursive protection
58+
/// feature is not enabled
59+
pub fn new(_min_stack_size: usize) -> Self {
60+
Self
61+
}
62+
}
63+
}

0 commit comments

Comments
 (0)