Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making the recursive dependency an optional feature #13766

Closed
Tracked by #13334
andygrove opened this issue Dec 13, 2024 · 11 comments · Fixed by #13778
Closed
Tracked by #13334

Making the recursive dependency an optional feature #13766

andygrove opened this issue Dec 13, 2024 · 11 comments · Fixed by #13778
Assignees
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

Is your feature request related to a problem or challenge?

The addition of the recursive crate as a dependency in #13310 may cause issues for some downstream projects.

For example, in the DataFusion Comet subproject, we started to see CI failures when running with miri.

error: unsupported operation: can't call foreign function `rust_psm_stack_pointer` on OS `linux`
    --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/psm-0.1.24/src/lib.rs:319:14
     |
319  |     unsafe { rust_psm_stack_pointer() }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function `rust_psm_stack_pointer` on OS `linux`
  

For now, we disabled the miri checks, but we would prefer to keep them and opt out of the recursive feature when testing in CI.

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

@berkaysynnada
Copy link
Contributor

I have experienced a similar problem for wasm, #13513. After disabling it, got no problem.

@alamb
Copy link
Contributor

alamb commented Dec 14, 2024

@blaginin has a PR to add recursive to sqlparser-rs as well:

Perhaps we can follow his model for making this feature optional in DataFusion

@alamb alamb changed the title Consider making the recursive dependency an optional feature Making the recursive dependency an optional feature Dec 14, 2024
@alamb
Copy link
Contributor

alamb commented Dec 14, 2024

I have added this ticket to the the list of things we should fix before 44 release: #13334

@alamb
Copy link
Contributor

alamb commented Dec 14, 2024

FYI @peter-toth

@findepi
Copy link
Member

findepi commented Dec 14, 2024

Perhaps we can follow his model for making this feature optional in DataFusion

Sounds simple.

@berkaysynnada would you like to make a PR?

@berkaysynnada
Copy link
Contributor

cc @buraksenn

@peter-toth
Copy link
Contributor

peter-toth commented Dec 14, 2024

Seems like rust_psm_stack_pointer is already on this wishlist for miri: rust-lang/miri#2057

@buraksenn
Copy link
Contributor

Thanks for the heads up @berkaysynnada . Opened a PR to make it optional

@buraksenn
Copy link
Contributor

take

@blaginin
Copy link
Contributor

i’m curious if there are any cases when downstream can’t use recursive yet needs stack overflow protection 🤔 if so, we may need switch to iterative after all

@alamb
Copy link
Contributor

alamb commented Dec 16, 2024

i’m curious if there are any cases when downstream can’t use recursive yet needs stack overflow protection 🤔 if so, we may need switch to iterative after all

I think the challenge will be that converting algorithms to an iterative approach is non trivial effort and will likely complicate the code. So if people can't use recursive but need more efficient stack usage, perhaps they can contribute changes to help improve DataFusion in this area

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants