sqlalchemy-stubs icon indicating copy to clipboard operation
sqlalchemy-stubs copied to clipboard

Typed queries

Open rafales opened this issue 6 years ago • 19 comments

Goal of this PR is to type orm's Query objects. This will allow to type-check stuff like:

session.query(Employee)  # -> Query[Employee]
session.query(Employee).get(123)  # -> Employee
session.query(Employee, Invoice)  # -> Query[Tuple[Employee, Invoice]]
session.query(Employee, Invoice).all()  # -> List[Tuple[Employee, Invoice]]

rafales avatar Jun 05 '19 22:06 rafales

Ok, so currently this PR handles model classes and columns, while gracefully downgrading to Any for other cases.

One thing I'm struggling with is keyed access to tuples returned by query. Under the hood SQLAlchemy defines sqlalchemy.util._collections.AbstractKeyedTuple which is a private class.

Then it provides sqlalchemy.util.KeyedTuple which implements AbstractKeyTuple. The catch is that query doesn't really use KeyedTuple. Instead it uses dynamically-created sub-classes of _LW (which inherits from AbstractKeyedTuple).

  1. It feels kind of dirty to use sqlalchemy.util._collection.AbstractKeyTuple which is private for some reason.
  2. It feels wrong to use KeyedTuple because assert isinstance(row.one(), KeyedTuple) would break.
  3. Right now AbstractKeyedTuple is typed as generic, not sure if it brings anything to the table.
  4. This feels wrong: sqlalchemy.orm.query.Query[Tuple[main.User, builtins.int*, fallback=sqlalchemy.util._collections.AbstractKeyedTuple[Any]]]
  5. On the other hand we should use a fallback for a tuple, otherwise keyed access will break.
  6. Right now keyed access results in Any, I don't see how it would be possible to figure out keys properly in mypy.

rafales avatar Jun 07 '19 22:06 rafales

Decided to use AbstractKeyedTuple as it aligns with actual type used. Should be ready to review. CLA signed.

rafales avatar Jun 14 '19 15:06 rafales

It seems that this will produce Query[int] for something like session.query(Employee.id). I think however that sqlalchemy will also return a subclass of _LW for this query.

libre-man avatar Jun 16 '19 21:06 libre-man

@libre-man just checked, you are correct. I'll fix it today/tomorrow.

rafales avatar Jun 17 '19 07:06 rafales

Btw this should be good to go.

rafales avatar Jul 15 '19 17:07 rafales

@ilevkivskyi If I implement your requested changes will we be good to go on this?

ckarnell avatar Sep 11 '19 18:09 ckarnell

If I implement your requested changes will we be good to go on this?

I would first ask if original author @rafales is still interested in continuing the work on this. In either case a second round of review may be needed.

ilevkivskyi avatar Sep 11 '19 21:09 ilevkivskyi

Hi @rafales! Do you think you're going to get a chance to finish this up any time soon?

ckarnell avatar Sep 12 '19 02:09 ckarnell

Sorry guys, I lost track of GH for some time. I made @ckarnell a collaborator in the repo as he wants to finish this PR.

rafales avatar Sep 18 '19 12:09 rafales

Thanks @rafales ! I had some free time when I was recently posting on this PR which I don't anymore, so I'm not sure I'll be able to get to this ASAP, but I'll try to some time soon!

ckarnell avatar Sep 18 '19 13:09 ckarnell

Sorry for pinging, what is the status of this PR? Are you waiting on any input from me? Is anyone still willing to continue working on this?

ilevkivskyi avatar Oct 14 '19 13:10 ilevkivskyi

I can see, there are some conflicts in this PR.

Ishaan28malik avatar Oct 14 '19 14:10 Ishaan28malik

I think this is one of the single highest value things we can add to the stubs so I'd prefer if it didn't get closed, but I can't guarantee I'll have to to work on it soon.

ckarnell avatar Oct 14 '19 16:10 ckarnell

@ilevkivskyi It actually seems like you had the most context based on your review, and getting it up to speed might be non-trivial because I noticed it doesn't appear to quite work with the new semantic analyzer. Is there any chance you could take a stab at finishing this?

ckarnell avatar Oct 14 '19 16:10 ckarnell

Is there any chance you could take a stab at finishing this?

I may be busy next few weeks/months. If no one will finish this one during this time, then I can finish it myself.

ilevkivskyi avatar Oct 14 '19 17:10 ilevkivskyi

Hi all, I've merged master and addressed review in another PR to @rafales ' fork here. I seem to be unable to request reviewers on that PR for some reason - @ilevkivskyi and @rafales , do you two mind reviewing it for me?

ckarnell avatar Nov 29 '19 17:11 ckarnell

I will take a look at this before the end of the year.

ilevkivskyi avatar Dec 04 '19 17:12 ilevkivskyi

I will take a look at this before the end of the year.

Actually, it looks like I will not have time to work on this, sorry.

ilevkivskyi avatar Dec 09 '19 15:12 ilevkivskyi

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Apr 16 '22 21:04 CLAassistant