search

OHDSI / ResultModelManager

RMM is an R package designed to handle common ohdsi results data management functions by providing a common API for data model migrations and definitions
https://ohdsi.github.io/ResultModelManager/
Apache License 2.0
3 stars 3 forks source link

Should isRequired be a required filed in the model spec csv? #59

Open chrisknoll opened 3 months ago

chrisknoll commented 3 months ago

See: https://github.com/OHDSI/ResultModelManager/blob/ad706b4396d8285a8930c9b99ec09e927e403b00/R/DataModel.R#L774

isRequired (I'm assuming this defines nullable columns) is not a required field of the model spec. Should it be?

azimov commented 3 months ago

Is required is not required - personally I would like to remove away from data models having required or not required fields. It adds complexity for the advantage of saving a few bytes. I'm also not sure any packages are currently using it and it is perfectly valid to have null fields within the database. We can review this prior to a 1.0.0 release

chrisknoll commented 3 months ago

Ok, that's fair and as author of RMM your prerogative, but, it does raise the question about how to specify that a column should be NULL-able in the DDL when it calls generateSqlSchema.

azimov commented 3 months ago

Personally I dislike the use of this function (I think writing SQL is explicit, and we already have a translation layer to other languages) but it was added as a convenience because strategus modules need to upload results to something.