Repository: martsberger/django-pivot (language: python)

Issue:
Aggregation on annotated field fails if queryset ordered by other values + set default value
Hey Marts,

First of all thanks for the package, it simple but very useful :)

I have a couple of comments/questions:

**1. Aggregation on annotated field** 
I have noticed that you do not reorder the QuerySet before aggregation, which causes problems when aggregating on an annotated field, if it is already ordered by another field.

For example, consider the model:
```
class Blog(models.Model):
    date = models.DateField()
    category = models.CharField(max_length=50)
    content = models.TextField(default='')
    
    class Meta:
        order_by = ['date']
```
If I pivot on an annotated field like:
```
qs = Blog.objects.annotate(period=Trunc('date', 'year'))
pivot(qs, 'period', 'category', 'content', Count)
```
Aggregation does not work because the 'date' field must be part of the query's _group by_ for the ordering to work. 

Reordering fixes the problem, so this will work:
```
qs = Blog.objects.annotate(period=Trunc('date', 'year')).order_by('period')
pivot(qs, 'period', 'category', 'content', Count)
```
I think it would make sense to implement this in the package.

**2. Default value**
The default column value is `None` atm. Why not using 0? Alternatively could one set the default value at module level? Something like:
```
def _get_annotations(column, column_values, data, aggregation):
    value = data if hasattr(data, 'resolve_expression') else F(data)
    return {
        display_value: aggregation(Case(When(Q(**{column: column_value}), then=value), default=DEFAULT_VALUE))
        for column_value, display_value in column_values
    }
```

**3. Cumulative aggregation**
It would be fairly easy to allow for cumulative aggregation as well, which can be useful. It can be done using the window function (+distinct):
```
window = Window(aggregator(data), order_by=row)
```
It does not work on sqlite though...
I'd be happy to have a look at it if you think it could be useful.


Discussion / hints:
Thanks for your feedback, these are good comments!

**#1 Aggregating annotated field and order_by**

If it works to remove the ordering altogether, I could implement that, .e.g., if
```python
qs = Blog.objects.annotate(period=Trunc('date', 'year')).order_by()
pivot(qs, 'period', 'category', 'content', Count)
```
fixes the issue, I could  make the pivot function remove the ordering. I think this should work as the result of pivoting should be independent of order_by.

If a specific ordering is required in this case it's a bit trickier, because adding an ordering that isn't necessary in other cases would degrade performance in those cases.

**#2 Default value**
There is a difference between None and 0 that is meaningful. If I find a bunch of records to sum up and the sum is zero, I should return the value 0. If I look for records to sum up and find no records at all, this is different. If I report 0 in the second case I'm indicating I found records that sum to zero, while if I return None I'm indicating no records were found.

Granted, in some cases people prefer to have the number 0 even in the second case I described above. There are two available ways to achieve this
* Convert None to 0 after you get the result from `pivot`
* Use an aggregation that converts Null to 0 in the database (this might not be the greatest implementation):
```python
from django.db.models import Count
class CountDefaultZero(Count):
    template = 'COALESCE({}, 0)'.format(Count.template)

pivot(qs, 'period', 'category', 'content', CountDefaultZero)
```
Possibly allowing a default passed into the pivot function would be nicer:
```python
pivot(qs, 'period', 'category', 'content', Count, default=0)
```
or maybe it unnecessarily clutters the api. I'll give it some thought.

**#3 Cumulative aggregation**
Does MySQL support the window function or just postgres? I'm okay with having db specific extensions, just need to think about the right way to do it. Is it possible to pass in an aggregation that does this?

If you have an implementation in mind, I'd be interested in seeing it.
#1: order_by() works and it is better than using an arbitrary ordering.

#2: that's what I figured, and that's what Excel pivot does by default too (I think).

It's a valid argument for Sum, less so for e.g. Count. In practice, like in your shirt sales example, if a shirt of a particular style has not been sold, it will be represented with no row present in an inventory table. The total income for that style should ideally return 0eur.

3# I think that most SQL languages support the `OVER (PARTITION BY x ORDER BY y)` clause which is used by the Window function (see [here](https://dev.mysql.com/doc/refman/8.0/en/window-functions-usage.html) for MySql). I have not tested it though, I use Postgres.

You cannot pass it as an aggregation because you need to call `distinct()` afterwards.
Basically instead of:
```
qs.annotate(**{column: aggregator(data)})
```

you need to call:
```
qs.annotate(**{column: Window(aggregator(data), order_by: F(row).asc())}).distinct()
```
It also requires some kind of support for passing ordering on the row field. Let me know if you want something more concrete.
Opened https://github.com/martsberger/django-pivot/issues/9 to address point 2 above.

Fix patch:
diff --git a/Readme.rst b/Readme.rst
index 3c248af..dcf2eb3 100644
--- a/Readme.rst
+++ b/Readme.rst
@@ -197,7 +197,7 @@ The test suite is run by `Travis <https://travis-ci.org/martsberger/django-pivot
 with Django versions 1.11, 2.0 and 2.1 and backends sqlite, MySQL, and Postgres. If you
 want to run the test suite locally, from the root directory::
 
-    python runtests.py --settings=django_pivot.tests.test_sqlite_settings
+    python runtests.py
 
 That will use sqlite as the backend and whatever version of Django you have
 in your current environment.
diff --git a/django_pivot/pivot.py b/django_pivot/pivot.py
index 4972df6..f2c0c3c 100644
--- a/django_pivot/pivot.py
+++ b/django_pivot/pivot.py
@@ -5,7 +5,8 @@
 from django_pivot.utils import get_column_values, get_field_choices, default_fill
 
 
-def pivot(queryset, rows, column, data, aggregation=Sum, choices='auto', display_transform=lambda s: s, default=None, row_range=()):
+def pivot(queryset, rows, column, data, aggregation=Sum, choices='auto', display_transform=lambda s: s,
+          default=None, row_range=(), ordering=()):
     """
     Takes a queryset and pivots it. The result is a table with one record
     per unique value in the `row` column, a column for each unique value in the `column` column
@@ -16,14 +17,16 @@ def pivot(queryset, rows, column, data, aggregation=Sum, choices='auto', display
     :param column: string, name of column that will define columns
     :param data: column name or Combinable
     :param aggregation: aggregation function to apply to data column
-    :param display_transform: function that takes a string and returns a string
+    :param choices: specify 'minimum' if you want to do an extra database query to get only choices present in the data
+    :param display_transform: function that takes an object and returns a string
     :param default: default value to pass to the aggregate function when no record is found
     :param row_range: iterable with the expected range of rows in the result
+    :param ordering: option to specify how the resulting pivot should be ordered
     :return: ValuesQueryset
     """
     values = [rows] if isinstance(rows, str) else list(rows)
 
-    queryset = _get_queryset(queryset)
+    queryset = _get_queryset(queryset).order_by(*ordering)
 
     column_values = get_column_values(queryset, column, choices)
 


Test patch:
diff --git a/django_pivot/tests/pivot/models.py b/django_pivot/tests/pivot/models.py
index 5c52167..f8955d7 100644
--- a/django_pivot/tests/pivot/models.py
+++ b/django_pivot/tests/pivot/models.py
@@ -20,3 +20,6 @@ class ShirtSales(models.Model):
     units = models.IntegerField()
     price = models.DecimalField(max_digits=8, decimal_places=2)
     store = models.ForeignKey(Store, on_delete=models.CASCADE)
+
+    class Meta:
+        ordering = ['shipped']
diff --git a/django_pivot/tests/pivot/test.py b/django_pivot/tests/pivot/test.py
index c6b0912..5101ff0 100644
--- a/django_pivot/tests/pivot/test.py
+++ b/django_pivot/tests/pivot/test.py
@@ -5,7 +5,8 @@
 from itertools import chain
 
 from django.conf import settings
-from django.db.models import CharField, Func, F, Avg, DecimalField, ExpressionWrapper
+from django.db.models import CharField, Func, F, Avg, DecimalField, ExpressionWrapper, Count
+from django.db.models.functions import Trunc
 from django.test import TestCase
 try:
     from django.utils.encoding import force_text
@@ -172,8 +173,9 @@ def test_monthly_report(self):
         else:
             return
 
-        shirt_sales = ShirtSales.objects.annotate(**annotations).order_by('date_sort')
-        monthly_report = pivot(shirt_sales, 'Month', 'store__name', 'units', default=0)
+        shirt_sales = ShirtSales.objects.annotate(**annotations)
+        monthly_report = pivot(shirt_sales, 'Month', 'store__name', 'units', default=0,
+                               ordering=['date_sort'])
 
         # Get the months and assert that the order by that we sent in is respected
         months = [record['Month'] for record in monthly_report]
@@ -190,6 +192,12 @@ def test_monthly_report(self):
                                                                              int(ss.shipped.month) == int(month) and
                                                                              ss.store.name == name)))
 
+    def test_annotate_non_ordered_field(self):
+        qs = ShirtSales.objects.annotate(period=Trunc('shipped', 'month')).order_by()
+        result = pivot(qs, 'period', 'gender', 'style', Count)
+
+        self.assertEqual(len(result), 7)
+
     def test_pivot_with_default_fill(self):
         shirt_sales = ShirtSales.objects.filter(shipped__gt='2005-01-25', shipped__lt='2005-02-03')
 
diff --git a/runtests.py b/runtests.py
index 28d3267..fc38089 100644
--- a/runtests.py
+++ b/runtests.py
@@ -5,21 +5,12 @@
 
 def parse_args():
     parser = OptionParser()
-    parser.add_option('-s', '--settings', help='Define settings.')
-    parser.add_option('-t', '--unittest', help='Define which test to run. Default all.')
-    options, args = parser.parse_args()
+    parser.add_option('-s', '--settings', default='django_pivot.tests.test_sqlite_settings', help='Define settings.')
+    return parser.parse_args()
 
-    if not options.settings:
-        parser.print_help()
-        sys.exit(1)
-
-    if not options.unittest:
-        options.unittest = ['pivot']
-
-    return options
 
 if __name__ == '__main__':
-    options = parse_args()
+    options, tests = parse_args()
     os.environ['DJANGO_SETTINGS_MODULE'] = options.settings
 
     # Local imports because DJANGO_SETTINGS_MODULE needs to be set first
@@ -32,4 +23,4 @@ def parse_args():
 
     TestRunner = get_runner(settings)
     runner = TestRunner(verbosity=1, interactive=True, failfast=False)
-    sys.exit(runner.run_tests([]))
+    sys.exit(runner.run_tests(tests))