On Feb 1, 2014, at 10:32 AM, Robert Tasarz <[email protected]> wrote:
> On 01/31/2014 10:56 PM, Michael Bayer wrote: >> On Jan 31, 2014, at 2:49 PM, Robert Tasarz <[email protected]> wrote: >>> Hi, >>> >>> I'm almost sure this is a bug, but maybe I'm missing something obvious. >>> I've tested it with Python 3.3, SQLAlchemy 0.9.1, PostgreSQL 9.3 and >>> reduced the issue to the following code: >>> >>> Here's most relevant part of the exception: >>> Traceback (most recent call last): >>> File >>> "/srv/websites/sika/local/lib/python3.3/dist-packages/sqlalchemy/engine/base.py", >>> line 867, in _execute_context >>> context) >>> File >>> "/srv/websites/sika/local/lib/python3.3/dist-packages/sqlalchemy/engine/default.py", >>> line 388, in do_execute >>> cursor.execute(statement, parameters) >>> psycopg2.ProgrammingError: column "a_1.id" must appear in the GROUP BY >>> clause or be used in an aggregate function >>> LINE 1: SELECT p.id AS p_id, sum(i.value) AS sum1, a_1.id AS a_1_id,… >> When joined eager loading is used, if LIMIT is also applied as you have in >> that [:10], SQLAlchemy wraps the query to be limited inside of a subquery, >> so that the LEFT OUTER JOIN for the eager loading can safely load all >> related rows without being subject to the LIMIT. This wrapping doesn’t >> occur when GROUP BY is used - GROUP BY is not usually used in conjunction >> with loads of a full entity, as this is typically inefficient - it is >> usually used with a query that is only loading individual columns, and then >> if the query overall is to return entities, a JOIN against the GROUP BY as a >> subquery is used (see >> http://weblogs.sqlteam.com/jeffs/archive/2005/12/14/8546.aspx for a >> description of this). > > Yeah, I understand the process behind, as despite using SQLAlchemy > for quite some time already, I'm still mostly "thinking in SQL" > first. This time I've been building query in interactive shell and > in such cases I'm usually using something like [:4] to roughly see > if a query that SA produces does what I'm expecting. So it surprised > me that when I've finally got what I needed calling .all() on it > thrown an exception. yeah but you have to appreciate that “joined eager loading”, while it is a SQL thing, represents a fairly dramatic injection of automation into the query. It can’t do the “subquery” thing unconditionally since that would drive our users nuts with performance issues. As I said it seems like maybe group_by() should also be subject to the subquery thing. Feel free to add an issue that group_by() should be subject to the subquery wrapping, though I don’t have the resources to prioritize it right now. > That's why I've started digging through the > internet, but found nothing relevant, so posted here. > Regarding article you've linked. That's all nice and true, but when > I'm using ORM I'm expecting some performance penalty sometimes in > exchange for short and clean syntax abstracting away inner details. > This time for my particular case it is around 30%-40% which gives > ~60ms longer execution time, so I can live with it. Now let's > compare code snippets based on my example classes from the first > post (assuming no eager load and no exception thrown). > > Original: > sumq = func.sum(I.value).label('sum1') > result = sess.query(P, > sumq).outerjoin(P.items).group_by(P).order_by(sumq).all() > > Optimized: > subq = db.query(I._elem, > func.sum(I.value).label('sum1')).group_by(I._elem).subquery() > result = sess.query(P, > subq.c.sum1).outerjoin(subq).order_by(subq.c.sum1).all() > > As you can see the second case is quite noticeably longer and more > complicated, moreover it is "leaking abstraction" as it needs to use > I._elem which is an implementation detail how relations are built in > SQL (can it be rewritten without it?). SQL itself is a leaky abstraction. The blog post illustrates an intuitive pattern that’s in its opinion “wrong”, and suggests a more complex approach that is “correct”. SQLAlchemy doesn’t want to deny you the fun of writing SQL so we stay pretty close to it :). > >> perhaps the presence of GROUP BY should be added to the list of things that >> cause the automatic wrapping with joined eager loading to occur, though the >> current behavior has been this way for nearly 8 years and nobody’s asked for >> it before. > > I guess it should, after all a bug that unveils itself once every > eight years is still a bug ;). Should I file a bug report? sure but note that the subquery/joined loader thing is a really sticky point in the whole thing, as you can imagine we’ve had dozens of bugs over the years related to that fairly complex functionality so it remains to be seen what impact adding group_by to the mix would have, both for backwards compatibility as well as stability. It could be no big deal but a lot of testing would need to be done.
signature.asc
Description: Message signed with OpenPGP using GPGMail
