You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/02/24 23:19:58 UTC

[GitHub] [arrow] seddonm1 opened a new pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

seddonm1 opened a new pull request #9567:
URL: https://github.com/apache/arrow/pull/9567


   @alamb @andygrove @nevi-me 
   
   Based on the Rust Arrow sync call we discussed setting feature flags to allow use of DataFusion without having to pull in unnecessary dependencies.
   
   Here is an example applied to the crypto functions. It:
   
   - puts the crypto functions behind a feature flag called `crypto-functions` with the required dependencies tagged.
   - adds the `crypto-functions` to the `default` feature flag meaning it will be included in the default build.
   - adds tests for both states of the feature flag. Note that the cargo behavior of specifying conditions in the rust test framework is inverted (at least in my mind) so that you can only specify `ignore` not `include` rules. Practically this means to run the test with the feature flag you do: `#[cfg_attr(not(feature = "crypto-functions"), ignore)]` which is not ideal but functional.
   - currently I am throwing `Err(DataFusionError::Internal("requires compilation with feature flag: crypto-functions".to_string()))` errors which I think is a better user experience than `panic` based macros but this could easily be changed to `unimplemented!` with a similar message.
   - unfortunately we will probably need to execute `cargo test --no-default-features --features cli` AND `cargo test` in CICD to ensure the proper coverage (`--features cli` is required to compile).
   
   Thoughts?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788765256


   @elferherreraI I think the response above answers this? (https://github.com/apache/arrow/pull/9567#issuecomment-788256916)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r584973666



##########
File path: rust/datafusion/Cargo.toml
##########
@@ -40,9 +40,10 @@ name = "datafusion-cli"
 path = "src/bin/main.rs"
 
 [features]
-default = ["cli"]
+default = ["cli", "crypto_expressions"]

Review comment:
       By having them on by default, how do we reduce compile times?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] elferherrera commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
elferherrera commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788858359


   > I think that care should be taken to keep the dependencies sensible but ultimately if we are chasing some sort of SQL compatibility (i.e. Postgres) then having the standard set of SQL functions on by default seems the correct approach.
   
   Sorry, didn't read carefully the last bit. So I guess if we don't want crypto you can only select `--features cli` like Andrew suggested.
   
   I like the changes


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-785843165


   FYI @ovr  -- given you contributed the original crypto functions, do you have any thoughts about making them optional (on by default)?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788830518


   > If we can reduce the number of dependencies or move something out, I think that might be preferable if at all possible.
   
   @Dandandan I suggest the approach, long term, of putting extension packages (and their tests) in their own crates. I envision this as a two step process:
   
   1. Pull the functions into crates in the same repo, but still release them all at the same time
   2. Eventually pull the functions into some other repo which would free them to be released on whatever timeline was good. 
   
   The thing is that we don't have a huge amount of dev time to devote to this project, and I worry about the overhead of splitting into separate crates.
   
   However until the time we have time to do something better, I think feature flags is the minimal overhead way to make the dependencies optional. 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-785463022


   https://issues.apache.org/jira/browse/ARROW-11775


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788129010


   @andygrove @Dandandan  @elferherrera  what do you think of this approach? I really like it


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9567:
URL: https://github.com/apache/arrow/pull/9567


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r585028643



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -447,6 +446,26 @@ pub fn return_type(
     }
 }
 
+#[cfg(feature = "crypto_expressions")]
+macro_rules! invoke_if_crypto_expressions_feature_flag {
+    ($FUNC:ident, $NAME:expr) => {{
+        use crate::physical_plan::crypto_expressions;
+        crypto_expressions::$FUNC
+    }};
+}
+
+#[cfg(not(feature = "crypto_expressions"))]

Review comment:
       @elferherrera I tried this but unfortunately the compiler does not like it. The `#[cfg]` tag does not operate on `{..}` blocks.
   
   The end-state of this was my least-bad solution.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-786172573


   Thanks @alamb 
   
   I did try to wrap code blocks `{}` with the macro and that did not work. I will see if the compiler is clever enough to handle macros as I agree this gets pretty messy. 
   
   I am not sure if we will ever be able to get away from the warnings unless MANY annotations are added which makes the code really horrible. I will try a few more tests.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] elferherrera commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
elferherrera commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r585392500



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -447,6 +446,26 @@ pub fn return_type(
     }
 }
 
+#[cfg(feature = "crypto_expressions")]
+macro_rules! invoke_if_crypto_expressions_feature_flag {
+    ($FUNC:ident, $NAME:expr) => {{
+        use crate::physical_plan::crypto_expressions;
+        crypto_expressions::$FUNC
+    }};
+}
+
+#[cfg(not(feature = "crypto_expressions"))]

Review comment:
       @seddonm1 That's odd, because before posting that I did a small test and it did compile. 
   
   ```rust
   #[cfg(feature = "rand_calc")]
   use playground::myrand;
   
   macro_rules! testing_something {
       ($name:expr) => {
           #[cfg(not(feature = "rand_calc"))]
           {
               println!("No feature: {}", $name);
           }
   
           #[cfg(feature = "rand_calc")]
           {
               println!("Feature: {}", $name);
           }
       };
   }
   
   fn main() {
       let a = 3.0001f64.to_le_bytes();
       println!("{:?}", a);
   
       #[cfg(feature = "rand_calc")]
       {
           let res = myrand::with_rand::create_rnd();
           println!("{:?}", res);
       }
   
       testing_something!("Name");
   }
   ```
   
   I should have tested it with your code to see if it would compile as well




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] ovr commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-785844998


   > FYI @ovr -- given you contributed the original crypto functions, do you have any thoughts about making them optional (on by default)?
   
   Looks ok to me :)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788256916


   @Dandandan this was discussed at the last Arrow Rust sync call. 
   
   The intention was to allow reducing compile time of projects which depend on DataFusion - not the DataFusion project itself. 
   
   I think that care should be taken to keep the dependencies sensible but ultimately if we are chasing some sort of SQL compatibility (i.e. Postgres) then having the standard set of SQL functions on by default seems the correct approach.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-785472375


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=h1) Report
   > Merging [#9567](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=desc) (56c854b) into [master](https://codecov.io/gh/apache/arrow/commit/5bea62493d919dcb97ca0f22bcb7ebfc239cee25?el=desc) (5bea624) will **increase** coverage by `0.12%`.
   > The diff coverage is `89.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9567/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9567      +/-   ##
   ==========================================
   + Coverage   82.25%   82.38%   +0.12%     
   ==========================================
     Files         244      244              
     Lines       55685    56234     +549     
   ==========================================
   + Hits        45806    46329     +523     
   - Misses       9879     9905      +26     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `81.56% <ø> (+0.42%)` | :arrow_up: |
   | [...datafusion/src/physical\_plan/crypto\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2NyeXB0b19leHByZXNzaW9ucy5ycw==) | `52.45% <ø> (ø)` | |
   | [...ion-testing/src/bin/arrow-json-integration-test.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9iaW4vYXJyb3ctanNvbi1pbnRlZ3JhdGlvbi10ZXN0LnJz) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/writer.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL3dyaXRlci5ycw==) | `87.23% <50.00%> (-0.59%)` | :arrow_down: |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `77.00% <82.23%> (+7.37%)` | :arrow_up: |
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `83.84% <88.23%> (+10.02%)` | :arrow_up: |
   | [rust/parquet/src/arrow/array\_reader.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9hcnJheV9yZWFkZXIucnM=) | `77.61% <91.30%> (-0.02%)` | :arrow_down: |
   | [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `98.28% <100.00%> (+0.14%)` | :arrow_up: |
   | [rust/parquet/src/arrow/arrow\_reader.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9hcnJvd19yZWFkZXIucnM=) | `91.91% <100.00%> (+0.34%)` | :arrow_up: |
   | [rust/parquet/src/schema/types.rs](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9zY2hlbWEvdHlwZXMucnM=) | `89.54% <100.00%> (+0.02%)` | :arrow_up: |
   | ... and [16 more](https://codecov.io/gh/apache/arrow/pull/9567/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=footer). Last update [81e9417...56c854b](https://codecov.io/gh/apache/arrow/pull/9567?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] elferherrera commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
elferherrera commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r584929370



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -447,6 +446,26 @@ pub fn return_type(
     }
 }
 
+#[cfg(feature = "crypto_expressions")]
+macro_rules! invoke_if_crypto_expressions_feature_flag {
+    ($FUNC:ident, $NAME:expr) => {{
+        use crate::physical_plan::crypto_expressions;
+        crypto_expressions::$FUNC
+    }};
+}
+
+#[cfg(not(feature = "crypto_expressions"))]

Review comment:
       I think you can put the `#[cfg(feature)]` inside the macro so you dont have to maintain two funtions
   What do you think?
   
   ```
   macro_rules! invoke_if_crypto_expressions_feature_flag {
       ($FUNC:ident, $NAME:expr) => {
           #[cfg(not(feature = "crypto_expressions"))]
           {
               |_: &[ColumnarValue]| -> Result<ColumnarValue> {
                   Err(DataFusionError::Internal(format!(
                       "function {} requires compilation with feature flag: crypto_expressions.",
                       $NAME
                   )))
               }
           }
   
           #[cfg(feature = "crypto_expressions")]
           {
               use crate::physical_plan::crypto_expressions;
               crypto_expressions::$FUNC
           }
       };
   }
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] elferherrera commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
elferherrera commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-787061390


   @seddonm1 I think you dont need to put the `#[cfg(feature)]` to all your functions. As long as you only put it where you are going to use the module. Something like this [example](https://old.reddit.com/r/rust/comments/9zbxe3/putting_modules_and_extern_crates_behind_feature/).


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r585477092



##########
File path: rust/datafusion/Cargo.toml
##########
@@ -40,9 +40,10 @@ name = "datafusion-cli"
 path = "src/bin/main.rs"
 
 [features]
-default = ["cli"]
+default = ["cli", "crypto_expressions"]

Review comment:
       > By having them on by default, how do we reduce compile times?
   
   You can locally run the tests with `cargo test --features cli` or something, right?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#discussion_r584908792



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -447,6 +446,26 @@ pub fn return_type(
     }
 }
 
+#[cfg(feature = "crypto_expressions")]
+macro_rules! invoke_if_crypto_expressions_feature_flag {

Review comment:
       Nice!




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-788196901


   The features for cryptography functions looks good to me! Thanks @seddonm1 !
   
   I think it is a good idea to make some things optional, to be able to exclude certain features. I think the features introduce some more complexity to the build, as we introduce more variants that maybe could fail to compile or to fail passing test in some configuration.
   So, overall I think we should strive to keep the usage relatively straightforward and avoid too complex usage.
   
   If we can reduce the number of dependencies or move something out, I think that might be preferable if at all possible.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9567: ARROW-11775: [Rust][DataFusion] Feature Flags for Dependencies

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9567:
URL: https://github.com/apache/arrow/pull/9567#issuecomment-787554082


   Thanks @alamb and @elferherrera 
   
   I have incorporated both your ideas within the limitations of the rust compiler; namely:
   1. feature flag names if used like `#[cfg(feature = "crypto_expressions")]` lose type safety on the feature name so care needs to be taken.
   2. using the `cfg!` macro looks like it would work but requires the function to compile under both conditions which is not easy. 
   
   I have also updated the rust.yml CICD file and added tests for consistency.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org