[PATCH] D37360: Keep sqlalchemy session separate from database objects

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D37360#858537, @kristof.beyls wrote:

> I have felt before that LNT's connection and interaction with the database is too obscure in the code.
>  Therefore, I like this proposal, even if it means passing around session objects more.
>
> I've even wondered before if we shouldn't just get rid of SqlAlchemy and write sql directly to allow us to speed up some of the heavy queries.
>  That would make the interaction with the database in LNT's code clearer again, removing an obfuscating layer of indirection.
>  However, I do see that with now supporting 3 database engines (sqlite, postgres, mysql), we may not want to have to reinvent the wheel on handling the database differences that SqlAlchemy already handles.
>  Anyway, the SqlAlchemy thought is clearly independent of this particular change.
>
> I've been out of the loop on python & database interactions for too many years to be able to properly review this, but as said above, I do like the direction this is taking LNT's design/code base.


Thanks for the review. Chris wanted to see this change too when I talked with him yesterday so I'm going ahead and commit it.

As for the rest: I feel the same way. I also think we would be better off with a set of helper functions to construct SQL queries rather than a full blown ORM layer. While doing simple things with sqlalchemy is simple enough, getting really accustomed to it and knowing how to debug and create performant queries takes weeks. This is unfortunate as most potential LNT developers will have a background in compiler technology and not in database programming.

That said I won't be the person rewriting all the code to use plain SQL (or some lower level abstraction as sqlalchemies DDL without using the full blown ORM); I'm also sure we can get the existing code working well so sqlalchemy is mainly a higher entry barriers for developing LNT (at least if someone wants to make fundamental changes).


Repository:
  rL LLVM

https://reviews.llvm.org/D37360