Re: [PR] fix: Enable WASM compilation by making sqlparser's recursive-protection optional [datafusion]

2025-06-17 Thread via GitHub 


alamb commented on PR #16418:
URL: https://github.com/apache/datafusion/pull/16418#issuecomment-2982051446

   Thanks again @jonmmease 


-- 
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]

Re: [PR] fix: Enable WASM compilation by making sqlparser's recursive-protection optional [datafusion]

2025-06-17 Thread via GitHub 


alamb merged PR #16418:
URL: https://github.com/apache/datafusion/pull/16418


-- 
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]

Re: [PR] fix: Enable WASM compilation by making sqlparser's recursive-protection optional [datafusion]

2025-06-16 Thread via GitHub 


jonmmease commented on code in PR #16418:
URL: https://github.com/apache/datafusion/pull/16418#discussion_r2150164907


##
datafusion/core/Cargo.toml:
##
@@ -79,6 +81,9 @@ recursive_protection = [
 "datafusion-physical-optimizer/recursive_protection",
 "datafusion-sql/recursive_protection",
 ]
+# Enable sqlparser's default features for backward compatibility
+sqlparser_std = ["sqlparser/std"]
+sqlparser_recursive_protection = ["sqlparser/recursive-protection"]

Review Comment:
   Oh, yeah, good call.  Made these updates in 
https://github.com/apache/datafusion/pull/16418/commits/485a5053f4750b8b83dd6c5a9a47dbfb7b5b3c3a



-- 
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]

Re: [PR] fix: Enable WASM compilation by making sqlparser's recursive-protection optional [datafusion]

2025-06-16 Thread via GitHub 


alamb commented on code in PR #16418:
URL: https://github.com/apache/datafusion/pull/16418#discussion_r2149552742


##
datafusion/core/Cargo.toml:
##
@@ -79,6 +81,9 @@ recursive_protection = [
 "datafusion-physical-optimizer/recursive_protection",
 "datafusion-sql/recursive_protection",
 ]
+# Enable sqlparser's default features for backward compatibility
+sqlparser_std = ["sqlparser/std"]
+sqlparser_recursive_protection = ["sqlparser/recursive-protection"]

Review Comment:
   Is there any need to add this new `sqlparser_recursive_protection = 
["sqlparser/recursive-protection"]` feature? could we instead just add it to 
the recursive_protection existing feature?
   
   ```toml
   recursive_protection = [
   "datafusion-common/recursive_protection",
   "datafusion-expr/recursive_protection",
   "datafusion-optimizer/recursive_protection",
   "datafusion-physical-optimizer/recursive_protection",
   "datafusion-sql/recursive_protection",
   "sqlparser/recursive-protection", # Add this???
   ]
   ```
   
   I also don't fully understand why we need a new feature for sqlparser_std as 
that would seem to already be enabled



-- 
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]

[PR] fix: Enable WASM compilation by making sqlparser's recursive-protection optional [datafusion]

2025-06-15 Thread via GitHub 


jonmmease opened a new pull request, #16418:
URL: https://github.com/apache/datafusion/pull/16418

   ## Summary
   
   This PR fixes the WASM compilation issue (#13513) by making sqlparser's 
`recursive-protection` feature optional. This allows DataFusion to be compiled 
for WebAssembly targets without encountering the "LLVM error: section too 
large" error caused by the PSM (Portable Stack Manipulation) dependency.
   
   ## Background
   
   When compiling DataFusion for WASM (target `wasm32-unknown-unknown`), the 
build fails with:
   ```
   error: failed to build archive at `.../libpsm-xxx.rlib`: LLVM error: section 
too large
   ```
   
   The dependency chain causing this issue is:
   ```
   datafusion → datafusion-sql → sqlparser (with default features) → recursive 
→ stacker → psm
   ```
   
   The PSM crate is incompatible with WebAssembly because.
   
   As noted in the [PSM 
documentation](https://github.com/rust-lang/stacker/blob/master/psm/README.mkd) 
itself:
   > "This library is not applicable to the target. WASM hasn't a specified C 
ABI, the callstack is not even in an address space and does not appear to be 
manipulatable."
   
   ## The Fix
   
   This PR implements a solution that maintains backward compatibility while 
enabling WASM builds:
   
   1. **Workspace Level**: Set `default-features = false` for sqlparser to 
prevent automatic inclusion of `recursive-protection`
   2. **DataFusion Core**: Add explicit feature flags for sqlparser's default 
features:
  - `sqlparser_std` → enables `sqlparser/std`
  - `sqlparser_recursive_protection` → enables 
`sqlparser/recursive-protection`
   3. **Default Features**: Include these flags in DataFusion's default 
features to maintain backward compatibility
   
   ## Usage
   
   After this change:
   
   ### For WASM builds (no PSM dependency):
   ```toml
   datafusion = { version = "48.0.0", default-features = false, features = 
["parquet", "sqlparser_std"] }
   ```
   
   ### For regular builds (unchanged behavior):
   ```toml
   datafusion = "48.0.0"  # Includes all default features including recursive 
protection
   ```
   
   ## Related Issues and Context
   
   - Fixes #13513: "recursive" Dependency Causes "section too large" Error When 
Compiling for wasm
   - Builds upon the work in PR #13887 which made recursive_protection optional 
in datafusion-sql
   
   ## Notes
   
   While PR #13887 made `recursive_protection` optional in datafusion-sql, it 
didn't fully solve the WASM compilation issue because sqlparser itself was 
still being pulled in with its default features across multiple DataFusion 
crates. This PR completes the fix by ensuring sqlparser's default features can 
be controlled at the workspace level.
   
   


-- 
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]