Added feature validation#258
Conversation
hangfei
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the contribution!
|
Once the tests passed, we can merge this change. |
|
build is failing for reason unrelated to change |
|
The one you shared seems to be warnings. The root cause is this failed test: I am re-running the tests to see if it's transistent. |
|
The feature name f_snowflake_call_center_division_name and f_snowflake_call_center_zipcode seems good. It should pass your validation. Doesn't seem to be related to your change. |
|
It seems the failure exist in main branch. Let me do some research to fix it. |
Sounds good. I will merge main branch into this PR branch once the main build is green. |
|
@lhayhurst I looked thru the test failures and they are unrelated with the changes that you made. I have a small PR for those (#276) in case you are interested in. I went ahead and merged the code. Thanks for the contribution! |
* main: (30 commits) Yihui/moderate registration conflict (#304) Update homepage (#310) Add extensible extractor APIs (#302) Remove Java and JS from Code Scanning Create codeql-analysis.yml [feathr] Add API to materialize features to offline store (#294) Improve error message when path is not supported (#257) Add tech talk slides for Feathr (#296) Update README.md Add milestone link (#286) Fix millisecond timestamp handling (#288) Consolidating CI pipelines (#280) Fixed dependecy problem of pretty print utils (#273) Fixing a broken link in README.md (#277) Fix test failure (#276) Added feature validation (#258) Feathr UI: Display feature key and transform expression in feature detail pages (#262) Feathr UI: enable multiple tenant auth (#266) Reduce feathr web api docker image build time (#261) Pretty-print the features produced by buildFeatures (#214) ...
See this issue, "Add validation for feature name".
This PR adds a new method to
feathr_project/feathr/feature.py,FeatureBase.validate_feature_name, that validates the provided feature name against the following rule:Unit tests are provided for this new function, which is called from the
FeatureBaseconstructor before setting theself.name.I drafted a (now closed) PR against my fork, lhayhurst#1, in order to validate that this PR was headed in the right directi. @hangfei gave me feedback in that PR, which I've incorporated here.