andygrove commented on code in PR #2373:
URL:
https://github.com/apache/datafusion-ballista/pull/2373#discussion_r3890144014
##########
ballista/executor/Cargo.toml:
##########
@@ -33,7 +33,7 @@ path = "src/bin/main.rs"
required-features = ["build-binary"]
[features]
-arrow-ipc-optimizations = []
+arrow-ipc-optimizations = ["ballista-core/arrow-ipc-optimizations"]
Review Comment:
Worth a sentence in the description I think. This does widen what the
executor's feature turns on, and cargo feature declarations are part of what a
library user pins against.
Someone doing this today:
```toml
ballista-core = { version = "54", default-features = false }
ballista-executor = { version = "54", default-features = false, features =
["arrow-ipc-optimizations"] }
```
is deliberately keeping validation on in core, and after this change they
quietly lose it. Given `with_skip_validation` is unsafe, that flip is safety
relevant rather than just a perf tweak.
I still think the change is right. The pre-existing half-on state is worse,
and making the name mean what it says is the whole point. It just makes "Are
there any user-facing changes? No" a bit too strong for that one config, and it
costs a line to say so.
--
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]
