Re: Window expressions, #26608

2017-02-24 Thread Mads Jensen
On 02/22/2017 12:00 PM, Josh Smeaton wrote:
> - I'm unsure how DEFAULT_INDEX_TABLESPACE is causing issues with your
> patch, can you explain a bit?

It appears that it's necessary to have functions defined
django/db/models/functions/X.py be importable from
django.db.models.functions, which means that they should be in
django/db/models/functions/__init__.py. I tried to experiment with this,
but I get a traceback about DEFAULT_INDEX_TABLESPACE not being set,
which again comes back Field referencing settings.DEFAULT_INDEX_TABLESPACE.

I had to revert the commit again:
https://github.com/django/django/pull/7611/commits
/0568902abadafa25fdc81b2ec5df50e5b7c11be7

> - Explicit arguments in functions are a nice to have, and definitely
> help when you know exactly what should and shouldn't be allowed, but are
> not required. For functions that take no arguments, it's better to
> override the `__init__` method and make that explicit. You could also
> define the `arity` class property if you know exactly how many
> expression arguments are needed for a Func.

The window functions are a bit of an odd case, as they are both
applicable in X() OVER(), but also in X() WITHIN GROUP(ORDER BY ...)
where X can take different arguments. I posted a couple links to some
blogs that explain this with a few examples. I'd like them also to
support WITHIN GROUP, where they can accept an argument; the
output-field is always for some functions either an integer or a float,
regardless of the input, and for others, such as last_value, it depends
on the type of the first argument, i.e., last_value(column with a
date-type) should output a date-type.

https://www.depesz.com/2014/01/11/waiting-for-9-4-support-ordered-set-within-group-aggregates/
https://sqlandplsql.com/2012/04/18/oracle-rank-function/

> - __str__ and __repr__ methods don't need to and shouldn't return
> database specific representations. They're there for debugging support
> mostly, and users aren't going to care (too much) if the PRECEDING
> language is slightly different from their particular backend. Hard code
> what you think will be the most useful representation in the __str__ and
> __repr__ methods. Tests are there to encourage implementors to add
> str/repr methods to their own expressions, that is all.

OK. Good.

> - are there other cases that should fail? Perhaps aggregating a window
> clause? I don't actually know, but it'd be worth testing. I think this
> will be caught by virtue of window functions deriving from Aggregate,
> but double check:
> 
> WindowTestModel.objects.annotate(lead=Window(
>   expression=Lead('salary'),
>   partition_by='department')
> ).aggregate(Sum('lead'))

This doesn't crash on Postgres, and sums the result of lead together. It
adds a group by, though.

I suppose an exception should also be raised in case there's no ORDER BY
inside the window, at least some of the tests failed on Oracle because
they didn't specify the ordering. For instance, Oracle wasn't thrilled
about ROW_NUMBER() OVER(), although Postgres could understand it (I
haven't yet installed MariaDB locally to get an idea what works and what
doesn't).

Thank you. I got a few clues to work from here, but I'll ask again at
some point (I spent quite a lot of time on this, so I want to see it
merged) :)

> Cheers
-- 
Med venlig hilsen / Kind regards,
Mads Jensen

 Saajan Fernandes: I think we forget things if there is nobody to
   tell them.
 -- The Lunchbox (2013)

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/7076baa1-78bd-c850-9abe-ed1346ad3e5b%40inducks.org.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2017-02-22 Thread Josh Smeaton
First off, great work on this patch. I haven't given it much of a look over 
just yet unfortunately, but I'm trying to figure out where your issues are 
to help unblock.

- I'm unsure how DEFAULT_INDEX_TABLESPACE is causing issues with your 
patch, can you explain a bit?

- Explicit arguments in functions are a nice to have, and definitely help 
when you know exactly what should and shouldn't be allowed, but are not 
required. For functions that take no arguments, it's better to override the 
`__init__` method and make that explicit. You could also define the `arity` 
class property if you know exactly how many expression arguments are needed 
for a Func.

- Overriding resolve_output_field is a fine idea, and we should probably 
make use of that a little better in other types. You could also just 
override an `output_field` property to extract the output_field of the 
first expression in self.get_source_expressions.

- django.db.models.functions.windows is fine

- __str__ and __repr__ methods don't need to and shouldn't return database 
specific representations. They're there for debugging support mostly, and 
users aren't going to care (too much) if the PRECEDING language is slightly 
different from their particular backend. Hard code what you think will be 
the most useful representation in the __str__ and __repr__ methods. Tests 
are there to encourage implementors to add str/repr methods to their own 
expressions, that is all.

- are there other cases that should fail? Perhaps aggregating a window 
clause? I don't actually know, but it'd be worth testing. I think this will 
be caught by virtue of window functions deriving from Aggregate, but double 
check:

WindowTestModel.objects.annotate(lead=Window(
  expression=Lead('salary'),
  partition_by='department')
).aggregate(Sum('lead'))

Happy to clarify anything above if I wasn't too clear.

Cheers

On Tuesday, 21 February 2017 17:36:04 UTC+11, Mads Jensen wrote:
>
> I hit somewhat of a wall, and would like some input/help.
>
> * Import of window functions from `django.db.model.functions` seems to be 
> the
>   convention - `DEFAULT_INDEX_TABLESPACE` causes some headaches, because of
>   XField being explicitly set in _output_field variable. Probably the 
> functions need some extra fine-tuning in some way. So
>   far, most of the functions don't have explicit arguments in the 
> constructor,
>   because they either have a varying number of arguments (Lead and Lag, for
>   instance) or don't take any (rank and row_number; however, those can 
> take an
>   argument in case somebody wants to add support for WITHIN 
> GROUP-expression). I
>   deleted the "_output_field = Field" from the functions that do take an
>   argument, such as Lead and Lag, would it be required to explicit about 
> the
>   output_field, such as overriding the resolve_output_field-field? In those
>   cases, it's always the type of the first argument or the 
> default-argument in
>   case it's provided/available. Tests are running locally without the 
> explicit
>   _output_field. And just to avoid shuffling around things later, any 
> arguments
>   against django.db.models.functions.window being the right place for this?
>
> * `__repr__` and `__str__` methods and testing. What's a good general 
> convention
>   for something like this? I don't have access to the connection in 
> __repr__,
>   and __str__-methods, and thus just imported from `django.db` in the 
> tests,
>   which I suppose is not the ideal solution for this. At least, it doesn't
>   really feel that way.
>
> * Are there more cases which should fail apart from filtering (supported by
>   filterable) and use in UPDATE (can_be_reffed_in_update)?
>
> Thank you.
>
> Kind regards,
> Mads
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/5662f3d4-619c-49df-ab02-efdeb8ee2573%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2017-02-20 Thread Mads Jensen
I hit somewhat of a wall, and would like some input/help.

* Import of window functions from `django.db.model.functions` seems to be 
the
  convention - `DEFAULT_INDEX_TABLESPACE` causes some headaches, because of
  XField being explicitly set in _output_field variable. Probably the 
functions need some extra fine-tuning in some way. So
  far, most of the functions don't have explicit arguments in the 
constructor,
  because they either have a varying number of arguments (Lead and Lag, for
  instance) or don't take any (rank and row_number; however, those can take 
an
  argument in case somebody wants to add support for WITHIN 
GROUP-expression). I
  deleted the "_output_field = Field" from the functions that do take an
  argument, such as Lead and Lag, would it be required to explicit about the
  output_field, such as overriding the resolve_output_field-field? In those
  cases, it's always the type of the first argument or the default-argument 
in
  case it's provided/available. Tests are running locally without the 
explicit
  _output_field. And just to avoid shuffling around things later, any 
arguments
  against django.db.models.functions.window being the right place for this?

* `__repr__` and `__str__` methods and testing. What's a good general 
convention
  for something like this? I don't have access to the connection in 
__repr__,
  and __str__-methods, and thus just imported from `django.db` in the tests,
  which I suppose is not the ideal solution for this. At least, it doesn't
  really feel that way.

* Are there more cases which should fail apart from filtering (supported by
  filterable) and use in UPDATE (can_be_reffed_in_update)?

Thank you.

Kind regards,
Mads

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/d81ddfe0-f8f2-4ec0-99c9-96cd4b619246%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2017-01-02 Thread Mads Jensen
I think this has cleaned up to request input (I'm a bit zealous to make 
something that will be merged). I left a note in 1.11.txt but when 2.0.txt 
is introduced, I'll move it there. Also, now everything is in places that 
are not PostgreSQL-only. I realized in the process that the only thing the 
list of functions in the features-list of a backend is to prevent the test 
suite to run tests for unsupported functions. Mostly because MariaDB won't 
include FIRST_VALUE and LAST_VALUE and a few more. Perhaps a single list 
would be better?

I have a few questions to clear this up before asking for a more formal 
review:

1. PostgreSQL and MariaDB don't include support for RESPECT|IGNORE NULLS 
for certain functions (details are provided in
https://www.postgresql.org/docs/current/static/functions-window.html). Any 
thoughts if adding ignore_nulls=False|True to the constructor would be an 
idea, and then specify it's only possible for Oracle, or "as_oracle" with a 
special template? I noticed that nulls_last and nulls_first was added to 
OrderBy, however that is valid in all implementations. The Oracle virtual 
box was too big to fit on my machine, so I haven't tested locally for more 
than PostgreSQL, and on Python 3.

2. About __repr__ and __str__, in general, I'm in doubt what would be good 
ways to represent things, and how to test them? Also, I am in doubt what 
are intuitive input values that translate to CURRENT ROW, UNBOUNDED 
FOLLOWING etc.? I had a look at sqlalchemy which uses a mix of null-values 
and integers as valid input. In general, the frames-part is a bit of 
headache, and still slightly messy (the generated SQL is fine, so I suppose 
the current test data is adequate).

And probably, django/db/backends/mysql/features.py should be left untouched?

I know 1.11 is in the works, so I'm fine with this being on hold.

Thank you,
~ Mads

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/822595ee-e75e-4979-8873-7b6488ee2d31%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-30 Thread Mads Jensen
On Tuesday, November 29, 2016 at 11:04:49 PM UTC+1, Josh Smeaton wrote:
>
> Adam, are you thinking we should be adding something like 
> Model.objects.window(), or just allowing Window-type expressions on 
> backends that have a specific feature flag? Does the compiler need to get 
> involved at all, or can we handle the full range of window expressions with 
> the expressions API?
>

I'd prefer the expression-api. I think it's more transparent what happens, 
and unless you're thinking of stating the windows explicitly after 
FROM/GROUP BY/HAVING, I don't see much reason.

I'm fairly new, but it seems to complicate things a lot more to need to 
write code inside the compiler, unless stricly necessary (supporting 
grouping sets could be one such example).
 

> Mads, there's nothing that currently handles a list of expressions, and 
> certainly nothing specific to OrderBy. Your proposed syntax is basically 
> what would be required `order_by=[ .. ]`.
>

I actually like the idea of introducing ExpressionList. It would provide a 
bit more power, such as offering ASC/DESC for each ordering expression. 
Anyway, I'll work a bit with this, and post some 
commits in the weekend. 

~ Mads

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/b78f508c-b3b2-456c-b88a-ee26dead4aae%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-30 Thread Mads Jensen


On Tuesday, November 29, 2016 at 10:35:34 PM UTC+1, Adam Johnson wrote:
>
> As to your point 1:
>  
>
>> Since this is specific to postgres
>
>
> Well, Window expressions aren't specific to Postgres:
>
>- They exist in Oracle
>- They exist in MariaDB 10.2 which is in beta and will be GA at around 
>the same time this patch is out: 
>https://mariadb.com/kb/en/mariadb/window-functions/
>
> I thus think this should all be part of Django's main QuerySet API.
>

This is also my plan. I left out an important "not" in that particular 
phrase. If you notice, I also added feature-flags. But still, the code is 
quite messy.

~ Mads 

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/4ff78da9-0265-4aeb-be37-eb456fdc13e6%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-29 Thread Shai Berger
On Wednesday 30 November 2016 03:10:23 Michael Manfre wrote:
> > On 29 November 2016 at 22:04, Josh Smeaton 
> > wrote:
> 
> > Mads, there's nothing that currently handles a list of expressions, and
> > certainly nothing specific to OrderBy. Your proposed syntax is basically
> > what would be required `order_by=[ .. ]`.
> 
> We (or anyone) could make an ExpressionList class that is essentially an
> Expression that is a list of expressions. It would basically be a
> Combinable with comma as the connector.
> 
I don't believe you can override comma, and I'm also not sure an 
ExpressionList is of general utility -- so far, all the places where we needed 
such lists were method calls (like order_by()). I'd try harder to look for 
APIs which keep it that way -- just off the top of my head,

Window(expression, *order_by)

(this probably doesn't work, Window() is more complex than that, but the 
order_by=[...] suggested by Mads probably does)


Re: Window expressions, #26608

2016-11-29 Thread Michael Manfre
On Tue, Nov 29, 2016 at 6:37 PM Adam Johnson  wrote:

> I'm not sure about that detail atm, I need to review the patch. I just
> think we shouldn't be putting anything in postgres-only land.
>
> On 29 November 2016 at 22:04, Josh Smeaton  wrote:
>
> Adam, are you thinking we should be adding something like
> Model.objects.window(), or just allowing Window-type expressions on
> backends that have a specific feature flag? Does the compiler need to get
> involved at all, or can we handle the full range of window expressions with
> the expressions API?
>
>
I agree that this should be behind a database feature and not locked away
in contrib.postgres or with a vendor check. Regarding the specifics, I
haven't had time to dig in to see how difficult it would be to add window
expressions to the expressions API, but that would be the way I'd like to
see it happen. Doing something like Model.objects.window() seems like it
would require getting the compiler involved and be more of a pain trying to
implement the various window expressions for each backend.


> Mads, there's nothing that currently handles a list of expressions, and
> certainly nothing specific to OrderBy. Your proposed syntax is basically
> what would be required `order_by=[ .. ]`.
>
>
We (or anyone) could make an ExpressionList class that is essentially an
Expression that is a list of expressions. It would basically be a
Combinable with comma as the connector.

Regards,
Michael Manfre

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAGdCwBt17KQdoR9daBRbDtaOy%2B7tH3a%3D1KJB-bd1sx6CQrQ7PA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-29 Thread Adam Johnson
I'm not sure about that detail atm, I need to review the patch. I just
think we shouldn't be putting anything in postgres-only land.

On 29 November 2016 at 22:04, Josh Smeaton  wrote:

> Adam, are you thinking we should be adding something like
> Model.objects.window(), or just allowing Window-type expressions on
> backends that have a specific feature flag? Does the compiler need to get
> involved at all, or can we handle the full range of window expressions with
> the expressions API?
>
> Mads, there's nothing that currently handles a list of expressions, and
> certainly nothing specific to OrderBy. Your proposed syntax is basically
> what would be required `order_by=[ .. ]`.
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/django-developers/f4999522-0a99-4e47-b499-
> bfba19cf3920%40googlegroups.com
> 
> .
>
> For more options, visit https://groups.google.com/d/optout.
>



-- 
Adam

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM3wTsNxkhSunvHefhgLGuwnTs6BAuQHtBsk%2Bcs-9H3FQQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-29 Thread Josh Smeaton
Adam, are you thinking we should be adding something like 
Model.objects.window(), or just allowing Window-type expressions on 
backends that have a specific feature flag? Does the compiler need to get 
involved at all, or can we handle the full range of window expressions with 
the expressions API?

Mads, there's nothing that currently handles a list of expressions, and 
certainly nothing specific to OrderBy. Your proposed syntax is basically 
what would be required `order_by=[ .. ]`.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/f4999522-0a99-4e47-b499-bfba19cf3920%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-29 Thread Adam Johnson
As to your point 1:
 

> Since this is specific to postgres


Well, Window expressions aren't specific to Postgres:

   - They exist in Oracle
   - They exist in MariaDB 10.2 which is in beta and will be GA at around 
   the same time this patch is 
   out: https://mariadb.com/kb/en/mariadb/window-functions/

I thus think this should all be part of Django's main QuerySet API.

On Tuesday, November 29, 2016 at 8:56:47 PM UTC, Mads Jensen wrote:
>
> On Saturday, November 26, 2016 at 1:51:05 AM UTC+1, Josh Smeaton wrote:
>>
>> OrderBy takes an expression - it doesn't have to be a single column. For 
>> example, this is valid:
>>
>> (Lower('last_name') + Lower('first_name)).desc() == 
>> OrderBy(Lower('last_name') + Lower('first_name), descending=True)
>>
>
> Yes. However, I was more interested in being able to add a list of 
> expressions. The OrderBy-class does not evaluate more than
> one expression. The Case-expression right above iterates over all of the 
> cases, and parses them. This is what I had in mind. I don't writing 
> something for it, but
> I'm trying to use as much of the existing API as possible.
>
> What I had in mind was offering the flexibility to write this (maybe 
> offering a list of OrderBy-clauses as valid input would be a way to 
> remediate this):
>
> I read over the Query-class code which has add_ordering.
>
> qs = WindowTestModel.objects.annotate(sum=Window(
> expression=Sum('salary'),
> order_by=[ExtractYear('hiredate'), F('department')])
> )
> print(qs.query)
>
> Kind regards,
>
> Mads
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/da1309ef-dadd-41ab-8332-6a93cdc66719%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-29 Thread Mads Jensen
On Saturday, November 26, 2016 at 1:51:05 AM UTC+1, Josh Smeaton wrote:
>
> OrderBy takes an expression - it doesn't have to be a single column. For 
> example, this is valid:
>
> (Lower('last_name') + Lower('first_name)).desc() == 
> OrderBy(Lower('last_name') + Lower('first_name), descending=True)
>

Yes. However, I was more interested in being able to add a list of 
expressions. The OrderBy-class does not evaluate more than
one expression. The Case-expression right above iterates over all of the 
cases, and parses them. This is what I had in mind. I don't writing 
something for it, but
I'm trying to use as much of the existing API as possible.

What I had in mind was offering the flexibility to write this (maybe 
offering a list of OrderBy-clauses as valid input would be a way to 
remediate this):

I read over the Query-class code which has add_ordering.

qs = WindowTestModel.objects.annotate(sum=Window(
expression=Sum('salary'),
order_by=[ExtractYear('hiredate'), F('department')])
)
print(qs.query)

Kind regards,

Mads

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/5aecd2cc-452c-43df-a0eb-b482220839eb%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-25 Thread Josh Smeaton
OrderBy takes an expression - it doesn't have to be a single column. For 
example, this is valid:

(Lower('last_name') + Lower('first_name)).desc() == 
OrderBy(Lower('last_name') + Lower('first_name), descending=True)

On Friday, 25 November 2016 23:22:58 UTC+11, Mads Jensen wrote:
>
> On Tuesday, November 22, 2016 at 11:50:17 PM UTC+1, Josh Smeaton wrote:
>
>> Thanks for picking this up. I've been wondering if Window expressions 
>> would be possible, and what limitations we might have to make based on our 
>> ORM.
>>
>
> I was fortunate that the reporter posted a snippet to work from :) 
> Django's ORM is quite flexible, and adaptable. I remember seeing a video 
> with Alex Gaynor where he complained about its shortcomings, but that 
> seemed to have been sorted out.
>
> https://www.youtube.com/watch?v=GxL9MnWlCwo
>  
>
>> > 1. Since this is specific to postgres, I'm looking for a better place 
>> to 
>> put the actual Window-expression class, as well as axillary helpers 
>> (Order By, Partition and the frame). 
>>
>> I would create a new file beside expressions.py called 
>> django/db/models/window.py.
>>
>>
> Seems like a good starting point. I have been contemplating that 
> grouping.py could be a good name as well, in case somehow would like to add 
> WITHIN GROUP, or support for grouping sets (a bigger fish, since that means 
> appending stuff after GROUP BY etc.)
> Anyway, I have some window.py locally.
>  
>
>> > 2. Tests, and if the initial test-cases seem fair? I took a small 
>> sample 
>> of test cases related to some made-up salaries in a few departments.
>>
>> Yes, it's a good start, but I haven't thoroughly reviewed them yet 
>> either. It'll be easier to do when there's a proper PR.
>>
>
> I wouldn't call it proper, but I posted a pull request, and marked it 
> [WIP].
>
> > 3. The Order By-clause proposed in the initial code in the code only 
>> leaves room for one column (no expressions).
>>
>> Wouldn't the existing OrderBy type be sufficient? Really though, 
>> windowing should support a list of expressions (then call asc() or desc() 
>> on them).
>>
>
> Maybe I'm reading the code wrong, but doesn't OrderBy just refer to a 
> column, or are types of Expression/ExpressionWrapper also valid input?
>  
>
>> > 4. Would it be ok to add code for this in the base backend?
>>
>> Yes, if it makes the implementation nicer/easier for django and 3rd party 
>> backends. If having the code in the backend prevents *users* from creating 
>> their own windowing functions or modifying existing ones, then that's 
>> probably not ok. Django, backends, and users all need to be able to 
>> construct their own windowing expressions. You can certainly share code in 
>> the backend though, with things like PRECEDING ROW etc.
>>
>  
> I'm cleaning up things in the pull request. At least elements that are 
> mandatory are there, such as docs, tests and some code.
>  
>
>> > I'm relatively new to Django internals (it's fun and educational 
>> hacking 
>> on them), and it took me some time to read through the code and 
>> documentation for the ORM, and there are many facets of it I can still 
>> understand.
>>
>> None of us understand all of the ORM, and we're more than happy to have 
>> more people helping out, so thanks! If you create a PR (even if it's not 
>> ready, just mark is [WIP] for Work In Progress) it'll be easier to comment 
>> on specific things within the code and see all of the commits tied 
>> together. You're also more likely to get feedback on a PR and some guidance 
>> there too.
>>
>
> Good. I tend to have several small things in my commits, but I'll try to 
> work on individual things in this PR.
>
> Thanks for input.
>
> Kind regards,
> Mads
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/1e339090-3602-4f86-bb15-7fadc5f7ef0c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Window expressions, #26608

2016-11-25 Thread Mads Jensen
On Tuesday, November 22, 2016 at 11:50:17 PM UTC+1, Josh Smeaton wrote:

> Thanks for picking this up. I've been wondering if Window expressions 
> would be possible, and what limitations we might have to make based on our 
> ORM.
>

I was fortunate that the reporter posted a snippet to work from :) Django's 
ORM is quite flexible, and adaptable. I remember seeing a video with Alex 
Gaynor where he complained about its shortcomings, but that seemed to have 
been sorted out.

https://www.youtube.com/watch?v=GxL9MnWlCwo
 

> > 1. Since this is specific to postgres, I'm looking for a better place to 
> put the actual Window-expression class, as well as axillary helpers 
> (Order By, Partition and the frame). 
>
> I would create a new file beside expressions.py called 
> django/db/models/window.py.
>
>
Seems like a good starting point. I have been contemplating that 
grouping.py could be a good name as well, in case somehow would like to add 
WITHIN GROUP, or support for grouping sets (a bigger fish, since that means 
appending stuff after GROUP BY etc.)
Anyway, I have some window.py locally.
 

> > 2. Tests, and if the initial test-cases seem fair? I took a small sample 
> of test cases related to some made-up salaries in a few departments.
>
> Yes, it's a good start, but I haven't thoroughly reviewed them yet either. 
> It'll be easier to do when there's a proper PR.
>

I wouldn't call it proper, but I posted a pull request, and marked it [WIP].

> 3. The Order By-clause proposed in the initial code in the code only 
> leaves room for one column (no expressions).
>
> Wouldn't the existing OrderBy type be sufficient? Really though, windowing 
> should support a list of expressions (then call asc() or desc() on them).
>

Maybe I'm reading the code wrong, but doesn't OrderBy just refer to a 
column, or are types of Expression/ExpressionWrapper also valid input?
 

> > 4. Would it be ok to add code for this in the base backend?
>
> Yes, if it makes the implementation nicer/easier for django and 3rd party 
> backends. If having the code in the backend prevents *users* from creating 
> their own windowing functions or modifying existing ones, then that's 
> probably not ok. Django, backends, and users all need to be able to 
> construct their own windowing expressions. You can certainly share code in 
> the backend though, with things like PRECEDING ROW etc.
>
 
I'm cleaning up things in the pull request. At least elements that are 
mandatory are there, such as docs, tests and some code.
 

> > I'm relatively new to Django internals (it's fun and educational hacking 
> on them), and it took me some time to read through the code and 
> documentation for the ORM, and there are many facets of it I can still 
> understand.
>
> None of us understand all of the ORM, and we're more than happy to have 
> more people helping out, so thanks! If you create a PR (even if it's not 
> ready, just mark is [WIP] for Work In Progress) it'll be easier to comment 
> on specific things within the code and see all of the commits tied 
> together. You're also more likely to get feedback on a PR and some guidance 
> there too.
>

Good. I tend to have several small things in my commits, but I'll try to 
work on individual things in this PR.

Thanks for input.

Kind regards,
Mads

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/843eb618-7e8a-411e-aaa9-4efe5edce481%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Window expressions, #26608

2016-11-22 Thread Mads Jensen
Hi,

I got somewhat stuck on progress with this ticket, and as I'd like to
get it merged eventually (and avoid an abundant amount of fixes), I have
a few things I like a bit of input about.

1. Since this is specific to postgres, I'm looking for a better place to
put the actual Window-expression class, as well as axillary helpers
(Order By, Partition and the frame).
django/db/models/sql/expressions.sql may be one such place, but perhaps
introducing another file for this? Eventually, something like WITHIN
GROUP could be added, which shares some traits with window expressions.
This also goes the tests.

2. Tests, and if the initial test-cases seem fair? I took a small sample
of test cases related to some made-up salaries in a few departments. I
tried to introduce some wrapper to guard against testing functions that
aren't available (Oracle is the most complete in this sense, PostgreSQL
doesn't support all of the aggregate-functions, such as Variance).

3. The Order By-clause proposed in the initial code in the code only
leaves room for one column (no expressions). A regular query has
"add_ordering" to add more than one ordering-clause to a query, but this
just works for an ordering in SELECT, and I didn't spot anything that
could be used for this. Maybe abstracting the code from add_ordering so
it could be used also for something more? Something similar goes for
Partition By. Again, I'm sure it's a common use case to partition and
order by more than one expression. I currently commented out the
OrderByWrapper.

4. Would it be ok to add code for this in the base backend? I'm thinking
of constants for CURRENT ROW etc. which are shared among backends. I had
a look at SQLalchemy to see how things are arranged there, and what it
supports. SQLalchemy is feature-rich, but leaves out more
expressive-style ranges, and only supports unpreceding following/current
row, unbound following. I guess this is a reasonable limitation to make.

5. Will a 2.0 release notes be introduced soon, as I'm certain I won't
manage to make the final PR before the 1.11 feature freeze as it needs
to be finished, reviewed and merged. I started some time ago, and had it
lying around till I understood window functions better.

As pointed out in #26608, I have some rough, drafty code that's quite
far from a PR. I'm more interested in input for the above.

I'm relatively new to Django internals (it's fun and educational hacking
on them), and it took me some time to read through the code and
documentation for the ORM, and there are many facets of it I can still
understand. If someone is willing to guide me a bit, I'd be quite
helpful. Thank you.
-- 
Med venlig hilsen / Kind regards,
Mads Jensen

 Saajan Fernandes: I think we forget things if there is nobody to
   tell them.
 -- The Lunchbox (2013)




-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/482d2667-dea7-fd17-e820-6d6a0be08ae4%40inducks.org.
For more options, visit https://groups.google.com/d/optout.