tests: Add method to compile insert statements - #39
Conversation
amotl
left a comment
amotl
left a comment
There was a problem hiding this comment.
Thanks. I've added just a few suggestions at your disposal.
| if isinstance(compiled, tuple): | ||
| return [compiled] | ||
| return compiled |
There was a problem hiding this comment.
This feels strange, but of course I don't have any clues about the API.
Maybe gradually accompany the code base with type hinting? With the SQLAlchemy dialect, it didn't happen from the beginning (the code base is dating back to Python 2.x), that's why we need to bring it in late:
There was a problem hiding this comment.
You are right, this is wrong, I wrote that way earlier when I didn't understand the return of .as_sql (django internal code is mostly untyped) and forgot about it.
I've push a fix, thanks for the catch 👌
|
|
||
|
|
||
| class SqlExtractor: | ||
| def compile_insert_sql(model, objs, using="default"): |
There was a problem hiding this comment.
Maybe add as a @staticmethod to the SqlCompiler class?
There was a problem hiding this comment.
I feel lukewarm about that
| return compiled | ||
|
|
||
|
|
||
| class SqlCompiler: |
There was a problem hiding this comment.
I don't know much about the Django object model: Is the SqlCompiler only suitable for supporting software tests within this package, or would it also have a purpose to be included into the package itself, to support software testing for all users of cratedb-django?
There was a problem hiding this comment.
Even if it's not suitable for production purposes, we could ship it in cratedb_django.testing, in the same spirit like other libraries support downstream testing with concise and practical utilities, e.g. numpy.testing, pandas.testing, etc.
There was a problem hiding this comment.
It should only be used for internal testing, users don't typically need to check whether the generated SQL is correct or not, that'd be our job.
To me it's just a nice util for us, it's internal API might change and I'm not even sure it's 100% correct, for example the insert compiler is only partially correct, afterwards django might add extra SQL like returning field1,field2,field3..., that's why I'd rather it to be "private" since I know how it works and where to use it.
Maybe in the future we can expose it but again, I highly doubt it. In the end the goal of using an ORM is not to look at SQL and this would defeat that purpose.
9fa631b to
14effd5
Compare
2 participants
Summary of the changes / Why this is an improvement
Adds an util function to be able to compile insert statements without hitting the db