Hi Scott

As we discussed this should be a RFC type PR and review, I have not make any comments on code what-so-ever, we can do that later. First I will add my thoughts on a couple of you questions above and then describe what I found during my experimentation.

Note that in ixdat classes, columns are typed according to their python type, and it is up to the backend to figure out how to translate between available types.

Agree completely.

This is the dbdiagram of the database that one can hopefully derive...

I've actually made that an exercise, as the DBML language that dbdiagram.io uses is very simple, it was relatively straight forward for translate the knowledge I was able to extract into a diagram. More on that below.

Things left to do after this PR:

...
write an SQLite backend.

I would suggest breaking the last part off, because it can and it keep PR's smaller.

As far as the questions for the reviewer, I will give quick answers here and go in more detail below.

Is all the needed information there?

Almost I think. Some things, mostly centered around the naming of id columns end up being very implicit and mostly contained in the code the that generates the DB schema. I think that information really should be contained in the classes, but that I also think there are ways to do that.

Does the "inheritance" regime make sense, i.e. can it be translated to a database which is in sa?

I think it does. Again there are a few things that are a little implicit, which I think we should consider making explicit, but other that that then yes.

What happens with multiple inheritance? The trickiest thing in this PR was ECMSCalibration which inherits from ECCalibration and MSCalibration. However, its id is a foreign key that can only refer to one of ec_calibrations.id and ms_calibrations.id (even though both of those are foreign keys refering to calibrations.id). Ideally we'd have the same id appear in all thee tables ec_calibrations, ms_calibrations, and ecms_calibrations, and ecms_calibrations would only need the two columns id and L (working distance, the only attribute in an ECMSCalibration which isn't in an MSCalibration or ECCalibration). However, if the table-defining class attributes are also to be used to build the dictionary representations of objects, then everything needs to be there.

Yeah, I noticed this. Now, I'm embarrassed to say that for this problem I only read the beginning of the message above and thought I had al the information I needed, so I missed the point towards the end where you describe that the two ids are really the same. Thus, the idea I propose for this is not right on, but still contains what I consider are some sensible points and it can be adapted.

Details in the experimentation on the next comment...

I did a bunch of experimentation with this, because I wanted to really test whether all the necessary information was there. This experimentation (and some extra stuff) is the #83 PR. I added a new tool generate_db_schema.py to tools, which contains the code necessary build the DBML for ixdat's db schema. While making this experiment a few things fell out:

1. Some bug fixes, these relate primarily to classes where it was forgotten to set a new table name, when columns was changed compared to its base class[1] and some fixes related to certain tables references that didn't respect the table-names-are-plural-of-what-they-contain convention, which meant that the foreign-keys-are-named-as-the-singular-of-the-table-they-refer-to-suffixed-_id rule broke. Bug fixes for all there are in the PR.
2. There is something with the naming of id columns. I think this part is best illustrated by looking at the complexity of the code in generate_db_schema.py. Now, obviously, complicated code could also just mean that I didn't do a good enough job of refactoring and cleaning up :| but I think the core problem does contain more complexity than is good and I think it is in the wrong place, so it'd probably be difficult to make all that go away. More on this below.
3. We need a way to collect all Saveable classes. I implemented that via a metaclass, which I think is a good solution because it follows inheritance, so inheriting classes don't need to worry about it (the obvious alternative implementations being a class decorator, which will need to be applied to every class or a manually kept list). The metaclass right now however does more that I think it should, because I also implemented some consistency checks there (essentially some of the rules we discussed), but I'm unsure if I think they belong there. If you want check on class variables at definition time, I think it will have to be done there, but I think there would be other ways to do this which are better.

On the naming of id columns

As you can see, quite a bit of the complexity in the tool actually centers around the naming of id columns and figuring out what the id column was named in another table, for the cases where you need to name them as foreign keys. As you can see, if is possible to define rules for this, that ends up with a reasonable result, but in my view it puts too much of the logic inside the schema extractor. So, instead of all this, how about just explicitely listing the id columns, their name and which table.column_name they point to in case of foreign keys. Consider this:

columns = [
    Column("measurement_id", ctype=int, attribute_name_="id", primary_key=True, foreign_key=("measurements", "id"))
    Column("technique", ctype=str)
]

Doing it this way, although we provide some information, which is redundant, the name and the reference would be completely explicit and the only thing the schema builder would have to do was to check if the reference is valid. Besides from that, it keeps the locality of the information very high, which is something I have begun thinking a lot about: How much other code do I need to know about to read this line, and how far aways is it? In the existing implementation, you would need to have a look at the rules in builder to figure what all the columns would become, while reading the column spec, while in what I propose, it would be right there. This is BTW not really unique or my idea, you often see this kind of Column spec in ORM and the like. The point is of course, that we would want to implement as little of that stuff as we can get away with, but it might be necessary to expand it a little.

table_name and columns

We discussed the fact that we should now allow a table to define columns and not also change the table_name. Right now this rule is inforced in in the metaclass, because we sort of have to, because it deals with a coupling of two different class variables. A way around this, which you probably considered, is to replace all 4 variables that define the table with a TableSpec object. You would still, if you cared about it, need to check in the metaclass that a class that defined a table, actually sets the table name to something different from the super class, but at least we could use the TableSpec object as a way to force giving a table_name at the same time as you are defining new columns.

components

I skipped this one in schema builder. Basically, we end up in problems there, because the usual naming rules for id columns don't apply. I imagine that this table would have a multi column primary key (see below), like I did for the other linker tables, but we would need some way to specify the names. It could be a simple as extending the OwnedObjectList with an optional id_column_names property.

An interesting problem however is that I don't know if SQL allows you to form a two column primary key to foreign keys of the same column. If not, here we would need a separate throw away id column.

multi column primary key

So, SQL and as far as I can see, also sqlite, allows multi column primary keys. The rule for a tables primary key simply is that it has to be unique for every row. I think for linker tables that would always be the case, i.e. we would never have more than one link between the same two objects, so we might as well exploit that and avoid having to add a auto incrementing id column, purely to have a unique primary key. An additional bonus is that primary keys are always indexed, which means that you can do faster-than-linear searches in them, provided that you filter in the same order that the multiple columns in the primary key are given. I.e. since I imagine that a linker class from A to B, will result in a SQL search that says, "give me all the Bs for A", then the columns in the linker tables should list the A_id first.

Multiple inheritance

So, you mentioned the problems with the multiple inheritance classes. Since, as mentioned above, you can have multiple column primary keys, we could actually easily do real multiple inheritance in the DB schema. I made an example of this in the last commit of the PR. If you don't like it, I can just pop that off and you can merge the rest. However, in that feature I missed the last part of your comment where you mention that while the ECMS object, will link to two objects, each of those in term will actually have the same ID. So, I would propose a small change to my proposal. I still would suggest to make it possible to list two classes in the parent_table_class parameter, so that we can use it in algorithm like the ones in full_columns. That implementation BTW is not terribly good, but I ran out of time. Instead of just eliminating duplicate columns assuming that come from the same baseclass, we should walk the MRO to find the commen base and actually check. If you like the idea, I can fix it up. However, with regards to the ids, and your concern about them. Having already established a way to form the full columns, we only need to decide what to put in the table. Here I see two options, since we know that it is the same id, we could have just one column, and primary key, called shared_id, which just picks one of them (say the EC id). If however, we want to retain the option of finding all the EMCS calibrations in which a particular MS, then we could add the MS calibration id also, but just leave it out of the primary key.

I hope all of this makes sense and is useful. I have one more comment about DB normalization, but I think it is a point that we have been over before and that you ruled down, so I don't think it is important right now. I will add that later this evening, these comments should be sufficient to start work with it.

You anticipated almost all of my suggestions in comments to #83 in your comment above :)
I've merged #83 and will continue work here on our suggested changes, in approximately this order:

• Add an explicit foreign key specification for inheriting tables (your suggestion On the naming of id colums). I think that this can fix both the "data_serie_id" [sic] problem (Metaprogramming proposed changed #83 (comment)) as well as the multiple inheritance id (though in a different way than what you and I both came up with above respectively under Multiple inheritance and here: Metaprogramming proposed changed #83 (comment)

• Expand the metaclass functionality to replace the first few functions in generate_db_schema.py, as I suggested here: Metaprogramming proposed changed #83 (comment)

• Also look for OwnedObjectLists in inheriting tables, thus generating a field_axes table in the db schema. Pointed out here: Metaprogramming proposed changed #83 (comment)

• Expand the OwnedObjectList definition to enable custom naming of id's and multi-column primary keys (your suggestion above under multiple column primary keys). This will hopefully also solve the problem you point out above under components and get the field_axes table looking as specified here: Metaprogramming proposed changed #83 (comment)

Hopefully then we'll get exactly the desired db generation while simplifying the code. Whether we simplify the knowledge required of an ixdat developer to write a new Saveable class is another question. On the one hand, if possible, it might be nice if an experimental scientists could write their own Measurement class without knowing SQL. On the other hand, if that's not possible, better that someone who does know SQL will at least find it easy.

I think all of your comments about the route you intend to go with the implementation sounds good. Although I cannot completely wrap my head around all of them, since it has been a while since the initial review. I will have to review them as code when it is ready.

Hopefully then we'll get exactly the desired db generation while simplifying the code. Whether we simplify the knowledge required of an ixdat developer to write a new Saveable class is another question. On the one hand, if possible, it might be nice if an experimental scientists could write their own Measurement class without knowing SQL. On the other hand, if that's not possible, better that someone who does know SQL will at least find it easy.

True. OTOH if we wanted to make it easier to do, without adding all the bits that make it serializable, we could re-introduce something like what you commented on, with a secret _SKIP_IN_TABLE_CHECKS class variable and allude to it in the error message from the metaclass if the checks fail.

I've made it through the above list, and it's looking good :)
The dbdiagram it generates now looks complete to me:

Next things I can think of to do:

• move checks on duplicate column references from get_full_columns to metaclass
• likewise for owned objects
• go through code for readability and simplicity
• get MemoryBackend and DirectoryBackend working again with the new table spec system
• merge in from main
• table defs in the spectroscopy-based classes

Please comment on the content and order of above list!
Not sure when the best time is for a full code review?