FeatureSet - validation for entities and features

[george0st]

Hello @Hedingber, @katyakats , I have question about validation of names, types for features and entities. Do you see as good idea to add these validations to the methods add_features, add_entities?

In case of name validation, is it fine to check only these restrictions or others? I think:

• 't_' or '_'
• 'aggr_*'
• name length?

BTW:

• In current situation (without validation), it can generate issues in case of aggregation, etc.
• I attached a few "valid" samples which I tested in MLRun 0.7.1
  
  

[katyakats]

sounds good to me (after the release). @theSaarco what do you think?

[theSaarco]

👍 I like this proposal - from my knowledge of the code I'm not concerned about SQL injection, but "strange names" might pose issues when performing Spark queries, for example (column names may be invalid etc.). We should also add tests to check these things.
We should at a minimum enforce limitations on the names of the features/entities and the types of entities - Entity actually accepts value_type: ValueType as input but there's no validation at all on that.

[george0st]

@katyakats , I saw that for aggregation you change prefix from 't_' to '_'. Do you plan others changes (in near future) e.g. do you plan to split user columns and added system columns (based on internal processing or aggregation)?

@theSaarco , I would like to show, what can happend (e.g. SQL injection, etc.) but important is to check types also, because it can generate unexpected behavioral. I am not sure if it can affect also MySQL DB (as main repository for MLRun), but probably penetration tests can provide the answer.

[katyakats]

@george0st no