Issue: "KeyError: None" when using JOIN and aggregate_rows in 2.10.2
Author: eely22
State: closed
Created: 2018-05-14 15:33:22

There is a case where using joins and aggregate_rows is causing uncaught exceptions. The example seems to be related to joining tables through a junction table, where there are two tables A and C and another B that joins the two together. B has two rows, both pointing to the same A. The two B records have one with a C object and the other with a null C. Similar to this:

```
    B - C
   / 
A 
   \ 
    B - NULL
```

When attempting to join and aggregate the rows, we see the following stack trace:

```
Error
Traceback (most recent call last):
  File "/Users/ericely/workspace/test/test_peewee.py", line 48, in test_peewee
    print len(records)
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 3298, in __len__
    return len(self.execute())
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 2334, in __len__
    return self.count
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 2330, in count
    self.fill_cache()
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 2377, in fill_cache
    next(self)
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 2363, in next
    obj = self.iterate()
  File "/Users/ericely/workspace/env/lib/python2.7/site-packages/peewee.py", line 2761, in iterate
    instance._data[metadata.foreign_key.name]]
KeyError: None
```

The code to generate this is:

```
import unittest
from peewee import *

db = SqliteDatabase(':memory:')

# create a base model class that our application's models will extend
class BaseModel(Model):
    class Meta:
        database = db

class A(BaseModel):
    val = FloatField(default=3.14)

class C(BaseModel):
    val = IntegerField(default=42)

class B(BaseModel):
    a = ForeignKeyField(A, null=True, default=None)
    c = ForeignKeyField(C, null=True, default=None)

class Osha_Violation_Model_Tests(unittest.TestCase):

    def test_peewee(self):
        A.create_table()
        B.create_table()
        C.create_table()

        # save the first record chain
        A().save(force_insert=True)
        a = A.get(A.id == 1)

        C().save(force_insert=True)
        c = C.get(C.id == 1)

        B(a=a, c=c).save(force_insert=True)

        # save the second record chain, starting with the same A, without a C link
        a = A.get(A.id == 1)

        B(a=a).save(force_insert=True)

        records = A\
            .select(A, B, C)\
            .join(B, JOIN_LEFT_OUTER)\
            .join(C, JOIN_LEFT_OUTER)\
            .aggregate_rows()

        print len(records)
```

This is peewee 2.10.2. Even though 2.10.2 isn't the latest release, we are still using it since 3.X has some breaking backward changes. I know aggregate_rows was removed in 3.X and prefetch is now recommended, but we still have a relatively large codebase on 2.10.2.

Peewee 3.x removed `aggregate_rows` because it was, in retrospect, a poorly-designed hack. The concept, roughly, was to produce a cartesian product of all the requested data and then de-duplicate it on the Python side. It's slow (because Python), expensive (because amount of bits being xferred) and buggy (because I never trusted it enough to start using it in production).

For the trivial use-case it works fine (users and tweets). I should not have included it as a public API, though, and that mistake has been corrected in 3.x.

I'm not planning to implement any fixes for APIs that were deprecated in 3.x, unfortunately I just don't have either the time or the interest. I'm sorry that this bug is occurring, though, and would be happy to help you come up with an alternate way to implement what you're trying to do.

We are currently refactoring to use prefetch instead. However, I am struggling a bit to understand how it can replace certain parts of joining, such as ordering. So, for example, if I started with a query like this:
```
records = A\
            .select(A, B, C)\
            .join(B, JOIN_LEFT_OUTER)\
            .join(C, JOIN_LEFT_OUTER)\
            .order_by(C.val)\
            .aggregate_rows()
```

I would end up with a list of A objects ordered by some value in C, which is what I want. 

How would I do the same with prefetch? If I did this:

```
records = prefetch(A.select(), B, C.select().order_by(C.val))
```

I don't end up what I want, the list of A objects is ordered by A's primary key. 

I suppose I could order the list in python after I have fetched everything, but the database can do this for me and is more efficient. 

This can also apply to filtering results, so something like this:

```
records = A\
            .select(A, B, C)\
            .join(B, JOIN_LEFT_OUTER)\
            .join(C, JOIN_LEFT_OUTER)\
            .where(C.val < 100)\
            .aggregate_rows()
```

It is the same type of issue. I want a list of A objects filtered by some value from C. Again, I suppose I can filter in python. Or I could select the C objects I want and map it backwards, so like this:

```
prefetch(C.select().where(C.val < 100), B, A) 
```

But now I have a list of C objects and have to reverse my logic, which is unintuitive.

Is there any way to replicate the join behavior with prefetch? It uses subqueries, so I don't see how to do this. Or is there some other way I am not seeing? I am looking in the docs but can't seem to find any examples, so if you can point me to some, that would be appreciated.

So the thing is, you can actually just write:

```python
query = (A
         .select(A, B, C)
         .join(B, JOIN.LEFT_OUTER)
         .join(C, JOIN.LEFT_OUTER)
         .order_by(C.val))
for a in query:
    if a.b is None:
        # just an a
    elif a.b.c is None:
        # A and b, but no c
    else:
        # a b and c
```

The only thing is you'll get duplicate a's and b's potentially. All `aggregate_rows()` did was try to "roll up" the duplicates, but depending on you use-case you can handle it yourself.

As far as using `prefetch()`, you can also:

```python
a_s = (A
     .select()
     .join(B, JOIN.LEFT_OUTER)
     .join(C, JOIN.LEFT_OUTER)
     .order_by(C.val))
all_together = prefetch(a_s, B, C)
```

I see where you are going with this. The second answer sets off alarm bells in terms of efficiency as it is now doing multiple joins AND multiple subqueries. We have some more complex scenarios so we will need to profile this.

As for the first answer, again, we have some more complex queries then listed here, which could create a lot deeper nested logic. Something we will have to look at.

I get that the aggregate_rows isn't so efficient, but our workload is generally based on serverless computing. So we have infinite compute resources (so to speak), but the bottleneck is always the database. We try to offload as much as we can from the database, which is why aggregate_rows worked well for us. 

Either way, we have some options to stop using aggregate_rows. Perhaps we would also look into fixing the function to suit our purposes, we shall see.

Combining something like the first approach, which contains duplicate rows, with [itertools.groupby](https://docs.python.org/2/library/itertools.html#itertools.groupby) should allow you to emulate what aggregate_rows() was doing.

Just to clarify, you expect the second approach to also have duplicate A records, correct? So with either approach we will need to dedupe.

`prefetch` won't return duplicates, but it is structured differently.

prefetch won't, but the first query is a join, which will. I just tried it with my above example and saw it returned two objects even though there is only one A in the database.

Unless I am missing something?

If you're comfortable with SQL, I'd suggest running the query with the joins in a sql shell and observing the way the columns are represented (and duplicated).

I am comfortable SQL, and I did run the query. I understand how the columns are all represented. I guess my comment was that peewee is still returning me duplicate A objects, and I thought that you had indicated that maybe it would not. So I was trying to fish for a solution where I did not have to explicitly deduplicate in this scenario. I would honestly expect peewee to return me duplicate A objects, but thought your response indicated it would not. But maybe I just misunderstood your answer?

> peewee is still returning me duplicate A objects

Exactly. All `aggregate_rows()` did was to roll up the duplicates and accumulate joined rows as related instances. That's why I suggested using something like `itertools.groupby`.

For anyone who stumbles upon this, here is how we ultimately solved the issue:

```
a_s = (A
     .select()
     .join(B, JOIN.LEFT_OUTER)
     .join(C, JOIN.LEFT_OUTER)
     .order_by(C.val))
     .group_by(A.id)
all_together = prefetch(a_s, B, C)
```

Allows us to order A by joined tables, but we dedupe by grouping on A's primary key. Then we prefetch the other values through subqueries. So no duplicate A objects, and all other fields are prefetched. 

Performance can still be a concern as you are now joining multiple tables, grouping, and then running subqueries on top of that, so your mileage may vary depending on your situation.