From 397ccaa813d57f0b668451be36dc37b700d4f950 Mon Sep 17 00:00:00 2001 From: Lyle Hayhurst Date: Sun, 8 May 2022 13:09:50 -0700 Subject: [PATCH 1/5] Added feature name validation --- feathr_project/feathr/feature.py | 35 ++++++++++++++++-- .../test/test_feature_name_validation.py | 36 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 feathr_project/test/test_feature_name_validation.py diff --git a/feathr_project/feathr/feature.py b/feathr_project/feathr/feature.py index ac4b2b774..d1f9c3b44 100644 --- a/feathr_project/feathr/feature.py +++ b/feathr_project/feathr/feature.py @@ -1,13 +1,20 @@ -from abc import ABC, abstractmethod +import re from copy import deepcopy from typing import List, Optional, Union, Dict from jinja2 import Template from feathr.dtype import FeatureType -from feathr.transformation import ExpressionTransformation, Transformation, WindowAggTransformation -from feathr.typed_key import DUMMY_KEY, TypedKey from feathr.frameconfig import HoconConvertible +from feathr.transformation import ExpressionTransformation, Transformation +from feathr.typed_key import DUMMY_KEY, TypedKey + + +class FeatureNameValidationError(ValueError): + """An exception for feature name validation. + Feature names must consist of letters, number, or underscores, + and cannot begin with a number.""" + class FeatureBase(HoconConvertible): """The base class for features @@ -28,6 +35,7 @@ def __init__(self, key: Optional[Union[TypedKey, List[TypedKey]]] = [DUMMY_KEY], registry_tags: Optional[Dict[str, str]] = None, ): + FeatureBase.validate_feature_name(name) self.name = name self.feature_type = feature_type self.registry_tags=registry_tags @@ -45,6 +53,27 @@ def __init__(self, # self.key could be null, when getting features from registry. self.key_alias = [k.key_column_alias for k in self.key if k] + @classmethod + def validate_feature_name(cls, feature_name: str) -> bool: + """ + Only alphabet, numbers, and '_' are allowed in the name. + It can not start with numbers. Note that '.' is NOT ALLOWED! + """ + if not feature_name: + raise FeatureNameValidationError('Feature name rule violation: empty feature name detected') + + if str.isnumeric(feature_name[0]): + raise FeatureNameValidationError( + f'Feature name rule violation: feature name cannot start with a number; name={feature_name}') + + feature_validator = re.compile(r'^[a-zA-Z0-9_]+$') + + if not feature_validator.match(feature_name): + raise FeatureNameValidationError( + f'Feature name rule violation: only letters, numbers, and underscores are allowed in the name; name={feature_name}') + + return True + def with_key(self, key_alias: Union[str, List[str]]): """Rename the feature key with the alias. This is useful in derived features that depends on the same feature with different keys.""" diff --git a/feathr_project/test/test_feature_name_validation.py b/feathr_project/test/test_feature_name_validation.py new file mode 100644 index 000000000..fb0bff12e --- /dev/null +++ b/feathr_project/test/test_feature_name_validation.py @@ -0,0 +1,36 @@ +import string + +import pytest + +from feathr import FeatureBase, FeatureNameValidationError + + +@pytest.mark.parametrize('bad_feature_name', + [None, + '']) +def test_feature_name_fails_on_empty_name(bad_feature_name: str): + with pytest.raises(FeatureNameValidationError, match="empty feature name"): + FeatureBase.validate_feature_name(bad_feature_name) + + +def test_feature_name_fails_on_leading_number(): + with pytest.raises(FeatureNameValidationError, match="cannot start with a number"): + FeatureBase.validate_feature_name("4featurename") + + +def test_feature_name_fails_on_punctuation_chars(): + for char in set(string.punctuation) - set('_'): + with pytest.raises(FeatureNameValidationError, match="only letters, numbers, and underscores are allowed"): + FeatureBase.validate_feature_name(f"feature_{char}_name") + + +@pytest.mark.parametrize('feature_name', + ["feature_name", + "features4lyfe", + "f_4_feature_", + "_leading_underscores_are_ok", + "CapitalizedFeature" + '']) +def test_feature_name_validates_ok(feature_name: str): + assert FeatureBase.validate_feature_name(feature_name) + From 117e6ba63f54cd5a5762c465079e8a26c00718b8 Mon Sep 17 00:00:00 2001 From: Lyle Hayhurst Date: Sun, 15 May 2022 13:19:34 -0700 Subject: [PATCH 2/5] export exceptions by default. --- .../feathr/api/app/core/feathr_api_exception.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/feathr_project/feathr/api/app/core/feathr_api_exception.py b/feathr_project/feathr/api/app/core/feathr_api_exception.py index 0ced2340b..3c6be221e 100644 --- a/feathr_project/feathr/api/app/core/feathr_api_exception.py +++ b/feathr_project/feathr/api/app/core/feathr_api_exception.py @@ -1,8 +1,17 @@ -from app.core.error_code import ErrorCode - +from feathr.api.app.core.error_code import ErrorCode class FeathrApiException(Exception): def __init__(self,message:str,code:ErrorCode): self.code = code self.message = message + +class FeatureNameValidationError(ValueError): + """An exception for feature name validation. + Feature names must consist of letters, number, or underscores, + and cannot begin with a number. + Periods are also disallowed, as some compute engines, such as Spark, + will consider them as operators in feature name. + """ + + From 40ff954d5bddc47ea5498d9f81ebd63f931a6be6 Mon Sep 17 00:00:00 2001 From: Lyle Hayhurst Date: Sun, 15 May 2022 13:19:58 -0700 Subject: [PATCH 3/5] exporting exceptions by default. --- feathr_project/feathr/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/feathr_project/feathr/__init__.py b/feathr_project/feathr/__init__.py index 801c39b96..a3019bf0c 100644 --- a/feathr_project/feathr/__init__.py +++ b/feathr_project/feathr/__init__.py @@ -13,3 +13,4 @@ from .lookup_feature import LookupFeature from .aggregation import Aggregation from .feathr_configurations import SparkExecutionConfiguration +from feathr.api.app.core.feathr_api_exception import * \ No newline at end of file From 18c10850ed5147e16d0af461456b04143f64e505 Mon Sep 17 00:00:00 2001 From: Lyle Hayhurst Date: Sun, 15 May 2022 13:20:39 -0700 Subject: [PATCH 4/5] PR feedback: one regex for all checks, regex documentation --- feathr_project/feathr/feature.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/feathr_project/feathr/feature.py b/feathr_project/feathr/feature.py index d1f9c3b44..b46585993 100644 --- a/feathr_project/feathr/feature.py +++ b/feathr_project/feathr/feature.py @@ -4,18 +4,13 @@ from jinja2 import Template +from feathr.api.app.core.feathr_api_exception import FeatureNameValidationError from feathr.dtype import FeatureType from feathr.frameconfig import HoconConvertible from feathr.transformation import ExpressionTransformation, Transformation from feathr.typed_key import DUMMY_KEY, TypedKey -class FeatureNameValidationError(ValueError): - """An exception for feature name validation. - Feature names must consist of letters, number, or underscores, - and cannot begin with a number.""" - - class FeatureBase(HoconConvertible): """The base class for features It has a feature name, feature type, and a convenient transformation used to produce its feature value. @@ -57,20 +52,22 @@ def __init__(self, def validate_feature_name(cls, feature_name: str) -> bool: """ Only alphabet, numbers, and '_' are allowed in the name. - It can not start with numbers. Note that '.' is NOT ALLOWED! + It can not start with numbers. Note that '.' is NOT ALLOWED! + This is because some compute engines, such as Spark, will consider them as operators in feature name. """ if not feature_name: raise FeatureNameValidationError('Feature name rule violation: empty feature name detected') - if str.isnumeric(feature_name[0]): - raise FeatureNameValidationError( - f'Feature name rule violation: feature name cannot start with a number; name={feature_name}') - - feature_validator = re.compile(r'^[a-zA-Z0-9_]+$') + feature_validator = re.compile(r"""^ # from the start of the string + [a-zA-Z_]{1} # first character can only be a letter or underscore + [a-zA-Z0-9_]+ # as many letters, numbers, or underscores as you like + $""", # to the end of the string + re.X) if not feature_validator.match(feature_name): raise FeatureNameValidationError( - f'Feature name rule violation: only letters, numbers, and underscores are allowed in the name; name={feature_name}') + 'Feature name rule violation: only letters, numbers, and underscores are allowed in the name, ' + + f'and the name cannot start with a number. name={feature_name}') return True From 7d44555773c910253b02bc1237f40861c3a55080 Mon Sep 17 00:00:00 2001 From: Lyle Hayhurst Date: Sun, 15 May 2022 13:44:04 -0700 Subject: [PATCH 5/5] using import convention --- feathr_project/feathr/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/feathr_project/feathr/__init__.py b/feathr_project/feathr/__init__.py index a3019bf0c..7e74ef90d 100644 --- a/feathr_project/feathr/__init__.py +++ b/feathr_project/feathr/__init__.py @@ -13,4 +13,4 @@ from .lookup_feature import LookupFeature from .aggregation import Aggregation from .feathr_configurations import SparkExecutionConfiguration -from feathr.api.app.core.feathr_api_exception import * \ No newline at end of file +from .api.app.core.feathr_api_exception import * \ No newline at end of file