Issue: Peewee Not Propagating Empty Select (i.e., all columns) in certain scenarios
Author: logannc
State: closed
Created: 2018-01-24 23:29:50

I have not yet figured out why this is happening and there is a workaround that isn't very onerous, but I still thought I'd post this here.

Imagine you have a couple tables, `User` and `Post`.

**Post**

| id | user_id | created_at | content |
| -- | ---------- | -------------- | ---------- |
| int | int | timestamp | varchar |

**User**

| id | name |
| -- | -------- |
| int | varchar |

Imagine you wish to query a list of all Users and their latest post. This might look like:

```
PA = Post.alias()
max_sub = (
    PA
        .select(PA.user, peewee.fn.max(PA.created_at).alias('max_date'))
        .group_by(PA.user)
        .alias('max_sub')
)
latest_posts = (
    Post
        .select()
        .join(max_sub, on = (Post.user == max_sub.c.user_id) & (Post.created_at == max_sub.max_date))
        .alias('latest')
)
latest_posts_by_user = (
    User
        .select()
        .join(latest_posts, on = User.id == latest_posts.c.user_id)
)
```

However, `latest_posts_by_user` generates the following flawed SQL:

```
select u1.id, u1.name
from user as u1
    inner join (
        select p1.id
        from post as p1
            inner join (
                select p2.user_id, max(p2.created_at) as max_date
                from post as p2
                group by p2.user_id) as max_sub
                on ((p1.user_id = max_sub.user_id) and (p1.created_at = max_sub.max_date))
     ) as latest
     on (u1.id = latest.user_id)
```

Notice that this will fail because the `latest_posts` subquery has suddenly changed from selecting all columns to only selecting `id` - the join will fail because `latest.user_id` does not exist. This only occurs when it is a subquery. If you print out the sql for `latest_posts` outside of this context, it gives you the correct looking subquery selecting all columns in the Post table. 

A workaround is to simply specify in `latest_posts` which columns you need. This is not all that bad, but does defeat the purpose of what should be the equivalent of `select *`.

Peewee, for simplicity, will treat "default" calls to `select()` as selecting all columns declared on that model. However, when a "default" query is used for a subquery, Peewee will only select that table's primary key. To work around this, you can explicitly select the columns you desire.

Alternatively, could you write this as:

```python
PA = Post.alias()
subq = PA.select(PA.user, fn.MAX(PA.timestamp).alias('max_ts')).group_by(PA.user).alias('latest')
predicate = (Post.user == subq.c.user_id) & (Post.timestamp == subq.c.max_ts)
query = (Post
         .select(Post, User)
         .join(subq, on=predicate)
         .switch(Post)
         .join(User))

for result in query:
    print(result.user.username, result.content)
```

I ended up coding this example up and writing some tests around it, but the main issues with the queries you've written is that you have not been explicit about what columns / models you are SELECT-ing.

So `latest_posts`, since it is a "default" SELECT and is used as a sub-query (in the join), Peewee thinks you only need the "id". To remedy this, change the SELECT to:

```python
latest_posts = (Post
        .select(Post)  # Note I just added "Post", indicating we want all columns.
        .join(max_sub, on = (Post.user == max_sub.c.user_id) & (Post.created_at == max_sub.max_date))
        .alias('latest'))
```

Then, in the outermost query, we will want to select all the columns on the User table presumably, as well as the relevant fields from the Post query we joined on:

```python
latest_posts_by_user = (User
        .select(User, latest_posts.c.content, latest_posts.c.created_at)
        .join(latest_posts, on = User.id == latest_posts.c.user_id))
```

That being said, this revealed a limitation of the code that reconstructs the model graph when iterating over the cursor. Hence the patch in 930fbef.

Also note that the alternative query I proposed should work fine as well.

> However, when a "default" query is used for a subquery, Peewee will only select that table's primary key. 

This is what I was missing. IMO, this is an API bug as it leads to different behaviour/SQL generation based purely on the context of the query. 

If the behaviour is not changed, it should be more prominently mentioned as a drawback and/or the examples in the documentation should not demonstrate `select` with no parameters. For example, on http://docs.peewee-orm.com/en/latest/peewee/api.html#Model.select

> selection – A list of model classes, field instances, functions or expressions. If no argument is provided, all columns for the given model will be selected.

> Examples of selecting all columns (default):

`User.select().where(User.active == True).order_by(User.username)`

This, and all other examples, should be changed to use `User.select(User).where(...)...` so that the documentation demonstrates queries that are properly context independent, which I think is people's default intuition when reasoning about these python objects. 

If you think the behaviour should be changed, great. I might consider a PR but I'm not too familiar with the internals. Otherwise, I might take a stab at a PR updating the docs to phase out the empty select pattern.



> This is what I was missing. IMO, this is an API bug as it leads to different behaviour/SQL generation based purely on the context of the query.

It is this way by design, not by accident, since most-commonly if you are using a subquery you will just be interested in the primary key. Furthermore, how is peewee supposed to know which fields you wish to actually select from the subquery unless you specify them, in this example?

> since most-commonly if you are using a subquery you will just be interested in the primary key

I disagree with this or, at the very least, am not convinced of it - I don't think either of us has the data needed to make a claim one way or the other. I mean, the pattern I demonstrated above is a very common one, I think, when you have foreign keys (literal DB FKs or just joining on non-primary key fields).

It can't know which columns. Which is, IMO, why it should default to all columns which *does* include the primary key. Including only the primary key seems like a premature optimization that isn't worth the quirk of `select()` being context dependent.

Besides, what is the downside to defaulting to all columns everywhere, including subqueries? All queries should continue to work identically if they default to 'all columns' as they provide a superset of information. Even in the case I posed above, because I don't select the information in the outer most select none of the extra columns get sent over the network, the columns are just made available in the `on` predicate. Depending on the DB, the columns in the subquery select may not even be accessed by the DB if they aren't selected from the outer query or used in the `on` predicate - at least, I hope this occurs, it seems like a simple enough optimization.

> I don't think either of us has the data needed to make a claim one way or the other.

Well, I *have* been maintaining this project for 7 1/2 years, and this has always been the behavior of `select()`. I don't recall anyone else finding it confusing. Not that you're wrong -- but just that this hasn't struck anyone as confusing to the point of raising it as an issue.

> Besides, what is the downside to defaulting to all columns everywhere, including subqueries? 

Potentially inefficient or slow queries when only the ID column would suffice. Specifically being able to take advantage of primary-key or covering indexes, which is quite fast.

Typically I see subquery ID lookups when doing UPDATEs or DELETEs on databases that do not support JOINs on those types of queries.

It makes sense to me to optimize for the 80% case, but for the other 20% to follow a reasonable convention -- that is, explicitly specifying the columns you wish to select.

> Well, I have been maintaining this project for 7 1/2 years [...]

Fair enough. :) I only know my experience. Maybe most of your users have different usage patterns than me or structure their queries/databases differently. ¯\\\_(ツ)_/¯

> Potentially inefficient or slow queries when only the ID column would suffice. Specifically being able to 
take advantage of primary-key or covering indexes, which is quite fast.

I'm not sure what you mean by this. Merely selecting additional columns *shouldn't* slow a query down.

> Typically I see subquery ID lookups when doing UPDATEs or DELETEs on databases that do not support JOINs on those types of queries.

So, like, `Post.update(user = User.select().filter(name='Logan')).where(created_at > yesterday)` or something like that?

I guess I can see the utility in this case. Though, my (flawed) intuition about peewee had led me to write queries like `Post.update(user = User.select(User.id).filter(name='Logan')).where(created_at > yesterday)` because I *thought* that an empty select would default `select all_declared` and I knew that would be incorrect. :)

> It makes sense to me to optimize for the 80% case, but for the other 20% to follow a reasonable convention -- that is, explicitly specifying the columns you wish to select.

I absolutely agree with the sentiment, and I think I even agree with the defaulting to primary key in the update/delete case, but with `select()`, I think the 80% case is actually to select everything. But I'm not sure how technically feasible that is to split the update/delete case from the select case or if I'm going to be able to convince you :)

> Merely selecting additional columns shouldn't slow a query down.

https://www.sqlite.org/queryplanner.html#_covering_indices

For queries, let's say you want to delete tweets by users with inactive accounts and cannot do a JOIN.

```python
inactive = User.select().where(User.active == False)
Tweet.delete().where(Tweet.user.in_(inactive)).execute()
```

In general, it's always best to explicitly select what you need. I hardly ever use a bare call to `select()` except when testing stuff out in the shell. So the example in your update query is just fine because it's explicit.

> https://www.sqlite.org/queryplanner.html#_covering_indices

+1! Was not aware of these. Even so, if the extra columns are not used from the outer select, a where clause, or an on-predicate, the DB should be able to use the covering index still.

Like, I said, I agree that in the update/delete case, defaulting to just the primary key is more useful.

My problem is that `inactive` is now context dependent. It represents two different SQL queries based on where it is used. This makes it much more difficult to reason about. In the update/delete case, I think the productivity trade off is worth it because the alternative (select all) doesn't even make sense.

Anyway, if you don't think it's worth changing, I think it *is* worth updating the documentation to not use the naked select style `select()` or to make it more clear that it doesn't actually default to all columns all the time.