Disable crypto_expressions feature properly for --no-default-features
#10059
Conversation
github-actions
bot
added
physical-expr
There was a problem hiding this comment.
The same reasoning that applies to crypto_expressions would also technically apply to all of the default features in this crate. Should we remove them as well?
There was a problem hiding this comment.
I just went through the Cargo.toml files and discovered that the defaults are kinda b0rken in multiple places. I think another ticket would be in order - or else this ticket should be just titled 'fix the expression feature defaults to allow for proper disabling of features' and all the fixes done here.
There was a problem hiding this comment.
yes I think we should remove them as well - these are left overs
phillipleblanc
changed the title
crypto_expressions properlycrypto_expressions feature properly for --no-default-features
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @phillipleblanc and @Omega359 for the review
I agree that more cleanup in this area would be beneficial, but I also think this PR is a nice cleanup in itself. Thank you 🙏
There was a problem hiding this comment.
yes I think we should remove them as well - these are left overs
| [features] | ||
| crypto_expressions = ["datafusion-physical-expr/crypto_expressions"] | ||
| default = ["crypto_expressions", "regex_expressions"] | ||
| default = ["regex_expressions"] |
There was a problem hiding this comment.
even the regex_expressions doesn't make much sense here anymore
4 participants
Which issue does this PR close?
Closes #10058
Rationale for this change
Makes it possible to actually disable the crypto features if they aren't required for a user of DataFusion.
What changes are included in this PR?
In addition to the main change that fixes the issue above, I realized that the
crypto_expressionsfeature was added in two other crates that aren't using it anymore - so I removed them.Are these changes tested?
I tested that building with no default features does actually disable the crypto dependencies.
Are there any user-facing changes?
This could be a breaking change if people disabled the default features and were still using a crypto function. I will lean on the more experienced contributors to weigh in here.