Fix `recursive-protection` feature flag #13887

alamb · 2024-12-23T14:52:42Z

Which issue does this PR close?

Closes Making the recursive dependency an optional feature #13766

Rationale for this change

The recursive-protect flag was added in #13778, but when @andygrove tried to disable it in comet, recursive was still enabled.

I am not an expert in the crate feature mechanism, but the feature appears to get activated if a downstream crate like comet uses a crate like datafusion (the core crate) that passes the feature through

What changes are included in this PR?

Change feature flag to enable recursive protection at the main crate level
rename recursive-protection to recursive_protection to be consistent with other flags

Are these changes tested?

Make feature not enabled by default in sub crates
Add feature to the datafusion core crate
move another stack protection mechanism into a guarded section

Here is an example showing how I tested to verify that the feature flagging still works

$ cargo test --features=recursive-protection -p datafusion-common --lib | grep test_large_tree
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.08s
     Running unittests src/lib.rs (target/debug/deps/datafusion_common-3dd7d455d375513b)
$ cargo test -p datafusion-common --lib | grep test_large_tree
   Compiling datafusion-common v43.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/common)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1.32s
     Running unittests src/lib.rs (target/debug/deps/datafusion_common-a4d89ac32a58d472)
$

I also verified that the MIRI tests pass on apache/datafusion-comet#1154 with this fix

Are there any user-facing changes?

alamb · 2024-12-23T14:55:33Z

Given this experience, we really need a test for the recursive-protection feature flag / compiling datafusion with various feature flags. Maybe someone can figure out how to do so, following the model of arrow-rs:

https://github.com/apache/arrow-rs/blob/2c84f243b882eff69806cd7294d38bf422fdb24a/.github/workflows/arrow.yml#L63-L97

Maybe after @Omega359 's PR here we can add these tests

ci improvements, update protoc #13876

alamb · 2024-12-23T14:56:23Z

datafusion/common/Cargo.toml

@@ -36,7 +36,6 @@ name = "datafusion_common"
 path = "src/lib.rs"

 [features]
-default = ["recursive-protection"]


having the feature enabled by default in subcrates meant I think it basically was not possible to disable when using datafusion core crate

alamb · 2024-12-23T14:57:04Z

datafusion/core/Cargo.toml

@@ -69,6 +69,13 @@ pyarrow = ["datafusion-common/pyarrow", "parquet"]
 regex_expressions = [
    "datafusion-functions/regex_expressions",
 ]
+recusive-protection = [


This adds the recursive-protection feature to the datafusion (core) crate which then activates the feature in the subcrates

alamb · 2024-12-23T14:57:44Z

datafusion/sql/src/query.rs

@@ -62,10 +63,11 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                // The functions called from `set_expr_to_plan()` need more than 128KB
                // stack in debug builds as investigated in:
                // https://github.com/apache/datafusion/pull/13310#discussion_r1836813902
-                let min_stack_size = recursive::get_minimum_stack_size();


here is pretty good evidence that the feature flag is not working as expected, as this code requires recursive to compile but it wasn't failing the build -- I pulled the logic its own structure

Late LGTM. StackGuard is a very clever fix.

alamb · 2024-12-23T15:01:07Z

Also before I merge this I want to rename the flag to recursive_protection (underscore) to be consistent with the rest

alamb · 2024-12-23T15:52:21Z

datafusion/common/Cargo.toml

 avro = ["apache-avro"]
 backtrace = []
 pyarrow = ["pyo3", "arrow/pyarrow", "parquet"]
 force_hash_collisions = []
-recursive-protection = ["dep:recursive"]
+recursive_protection = ["dep:recursive"]


I also renamed the feature to use an underscore (_ to be consistent with the other feaures)

alamb · 2024-12-24T13:02:05Z

FYI @buraksenn and @peter-toth

xudong963

LGTM.

One concern is that some users may depend on the feature, so maybe we should highlight the change at the next release log.

alamb · 2024-12-24T15:31:44Z

LGTM.

One concern is that some users may depend on the feature, so maybe we should highlight the change at the next release log.

THis is a good point -- I believe 44.0.0 will be the first release that will have this feature -- 43 did not have it: https://github.com/apache/datafusion/blob/main/dev/changelog/43.0.0.md

andygrove

LGTM but I have not tested with Comet. Thanks @alamb

alamb · 2024-12-24T21:05:44Z

LGTM but I have not tested with Comet. Thanks @alamb

I have tested so locally -- specifically I checked out the code in apache/datafusion-comet#1154

Changed it to use a local checkout with this PR:

--- a/native/Cargo.toml
+++ b/native/Cargo.toml
@@ -39,15 +39,15 @@ arrow-buffer = { version = "53.3.0" }
 arrow-data = { version = "53.3.0" }
 arrow-schema = { version = "53.3.0" }
 parquet = { version = "53.3.0", default-features = false, features = ["experimental"] }
-datafusion = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false, features = ["unicode_expressions", "crypto_expressions"] }
-datafusion-common = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-functions = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false, features = ["crypto_expressions"] }
-datafusion-functions-nested = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-expr = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-expr-common = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-execution = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-physical-plan = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
-datafusion-physical-expr = { git = "https://github.com/apache/datafusion.git", rev = "242f45f", default-features = false }
+datafusion = { path = "/Users/andrewlamb/Software/datafusion/datafusion/core", default-features = false, features = ["unicode_expressions", "crypto_expressions"] }
+datafusion-common = { path = "/Users/andrewlamb/Software/datafusion/datafusion/common", default-features = false }
+datafusion-functions = { path = "/Users/andrewlamb/Software/datafusion/datafusion/functions", default-features = false, features = ["crypto_expressions"] }
+datafusion-functions-nested = { path = "/Users/andrewlamb/Software/datafusion/datafusion/functions-nested", default-features = false }
+datafusion-expr = { path = "/Users/andrewlamb/Software/datafusion/datafusion/expr", default-features = false }
+datafusion-expr-common = { path = "/Users/andrewlamb/Software/datafusion/datafusion/expr-common", default-features = false }
+datafusion-execution = { path = "/Users/andrewlamb/Software/datafusion/datafusion/execution", default-features = false }
+datafusion-physical-plan = { path = "/Users/andrewlamb/Software/datafusion/datafusion/core", default-features = false }
+datafusion-physical-expr = { path = "/Users/andrewlamb/Software/datafusion/datafusion/physical-expr", default-features = false }
 datafusion-comet-spark-expr = { path = "spark-expr", version = "0.5.0" }
 datafusion-comet-proto = { path = "proto", version = "0.5.0" }
 chrono = { version = "0.4", default-features = false, features = ["clock"] }

and verified that this command passes:

MIRIFLAGS="-Zmiri-disable-isolation" cargo +nightly miri test -p datafusion-comet -- test_unpack_dictionary_primitive

alamb · 2024-12-24T21:05:56Z

Thank you for the reviews @andygrove and @xudong963

Fix recursive-protection feature flag

18a0cff

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate common Related to common crate labels Dec 23, 2024

alamb commented Dec 23, 2024

View reviewed changes

alamb mentioned this pull request Dec 23, 2024

Release DataFusion 44.0.0 #13334

Closed

10 tasks

rename feature flag to be consistent

af34f73

alamb commented Dec 23, 2024

View reviewed changes

Make default

eaddc2b

github-actions bot added the documentation Improvements or additions to documentation label Dec 23, 2024

alamb mentioned this pull request Dec 23, 2024

[minor] make recursive package dependency optional #13778

Merged

taplo format

1e29b61

alamb requested a review from andygrove December 24, 2024 13:01

xudong963 approved these changes Dec 24, 2024

View reviewed changes

andygrove approved these changes Dec 24, 2024

View reviewed changes

alamb merged commit e99e02b into apache:main Dec 24, 2024
26 checks passed

alamb deleted the alamb/fix_recursive_protect branch December 24, 2024 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `recursive-protection` feature flag #13887

Fix `recursive-protection` feature flag #13887

alamb commented Dec 23, 2024 •

edited

Loading

alamb commented Dec 23, 2024

alamb Dec 23, 2024

alamb Dec 23, 2024

alamb Dec 23, 2024

peter-toth Dec 27, 2024

alamb commented Dec 23, 2024

alamb Dec 23, 2024

alamb commented Dec 24, 2024

xudong963 left a comment

alamb commented Dec 24, 2024

andygrove left a comment

alamb commented Dec 24, 2024

alamb commented Dec 24, 2024

Fix recursive-protection feature flag #13887

Fix recursive-protection feature flag #13887

Conversation

alamb commented Dec 23, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb commented Dec 23, 2024

alamb Dec 23, 2024

Choose a reason for hiding this comment

alamb Dec 23, 2024

Choose a reason for hiding this comment

alamb Dec 23, 2024

Choose a reason for hiding this comment

peter-toth Dec 27, 2024

Choose a reason for hiding this comment

alamb commented Dec 23, 2024

alamb Dec 23, 2024

Choose a reason for hiding this comment

alamb commented Dec 24, 2024

xudong963 left a comment

Choose a reason for hiding this comment

alamb commented Dec 24, 2024

andygrove left a comment

Choose a reason for hiding this comment

alamb commented Dec 24, 2024

alamb commented Dec 24, 2024

Fix `recursive-protection` feature flag #13887

Fix `recursive-protection` feature flag #13887

alamb commented Dec 23, 2024 •

edited

Loading