Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-20 Thread Dean Rasheed
On 16 March 2016 at 23:32, David Steele  wrote:
> On 3/10/16 1:24 PM, Corey Huinker wrote:
>
>> New patch for Alvaro's consideration.
>>
>> Very minor changes since the last time, the explanations below are
>> literally longer than the changes:
>> - Rebased, though I don't think any of the files had changed in the
>> mean time
>> - Removed infinity checks/errors and the test cases to match
>> - Amended documentation to add 'days' after 'step' as suggested
>> - Did not add a period as suggested, to remain consistent with other
>> descriptions in the same sgml table
>> - Altered test case and documentation of 7 day step example so that
>> the generated dates do not land evenly on the end date. A reader
>> might incorrectly infer that the end date must be in the result set,
>> when it doesn't have to be.
>> - Removed unnecessary indentation that existed purely due to
>> following of other generate_series implementations
>
>
> As far as I can see overall support is in favor of this patch although it is
> not overwhelming by any means.
>
> I think in this case it comes down to a committer's judgement so I have
> marked this "ready for committer" and passed the buck on to Álvaro.
>

So I was pretty much "meh" on this patch too, because I'm not
convinced it actually saves much typing, if any.

However, I now realise that it introduces a backwards-compatibility
breakage. Today it is possible to type

SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7 days');

and it works with just that one cast. However, this patch breaks that.

Now I'm not saying that I have used the above construct. Probably not
in fact, but maybe my work colleagues have. I honestly can't say,  but
the thought of having to grep through thousands of lines of
application code to check isn't particularly appealing.

So I'm afraid it's -1 from me.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread David Steele
On 3/17/16 11:55 AM, David G. Johnston wrote:

> On Thu, Mar 17, 2016 at 8:41 AM, David Steele  >wrote:
> 
>> Not sure I agree.  My point was that even if developers were pretty
>> careful with their casting (or are using two actual dates) then there's
>> still possibility for breakage.
> 
> With the first argument casted to date it doesn't matter whether you
> cast the second argument as the pseudo-type anyelement will take its
> value from the first argument and force the second to date.

Ah, I see.

> The main problem is that the system tries to cast unknown to integer
> based upon finding gs(date, date, integer) and fails without ever
> considering that gs(ts, ts, interval) would succeed.

I'm tempted to say, "why don't we just fix that?" but I'm sure any
changes in that area would introduce a variety of new and interesting
behaviors.

> ​So let the user decide whether to trade-off learning a new function
> name but getting dates instead of timestamps or going through the hassle
> of casting.​
> 
> For me, it would have been a nice bonus if the generate_series() could
> be used directly but I feel that the desired functionality is desirable
> and the name itself is of only minor consideration.
> 
> I can see that newbies might ponder why two functions exist but they
> should understand "backward compatibility".
> 
> I suspect that most people will simply think: "use generate_series for
> numbers and use generate_dates for, well, dates".  The ones left out in
> the cold are those wondering why they use "generate_series" for
> timestamp series with a finer precision than date.  I'd be willing to
> offer a "generate_timestamps" to placate them.  And might as well toss
> in "generate_numbers" for completeness - though now I likely doom the
> proposal...so I'll stick with just add generate_dates so the behavior is
> possible without superfluous casting or the need to write a custom
> function.  Dates are too common a unit of measure to leave working with
> them this cumbersome.

I'm not finding this argument persuasive.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Corey Huinker
On Thu, Mar 17, 2016 at 12:04 PM, Tom Lane  wrote:

> David Steele  writes:
> > On 3/17/16 11:30 AM, David G. Johnston wrote:
> >> ​I'd call it "generate_dates(...)" and be done with it.
> >> We would then have:
> >> generate_series()
> >> generate_subscripts()
> >> generate_dates()
>
> > To me this completely negates the idea of this "just working" which is
> > why it got a +1 from me in the first place.  If I have to remember to
> > use a different function name then I'd prefer to just cast on the
> > timestamp version of generate_series().
>
> Yeah, this point greatly weakens the desirability of this function IMO.
> I've also gone from "don't care" to "-1".
>
> regards, tom lane
>

Since that diminishes the already moderate support for this patch, I'll
withdraw it.


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Robert Haas
On Thu, Mar 17, 2016 at 12:59 PM, Tom Lane  wrote:
> One idea that might be worth considering is to define the function
> as generate_series(date,date,interval) returns timestamp (without
> time zone).  The point here would be only to move the behavior for
> date inputs as far as getting timestamp without tz rather than
> timestamp with tz; which would at least save some timezone rotations
> in typical use, as well as rather debatable semantics.  (The fact
> that timestamptz is the preferred type in this hierarchy isn't
> really doing us any favors here.)

That's a fairly tenuous benefit, though, and a substantially different
patch.  I think it's time to give up here and move on.  We can revisit
this for another release after we've had more time to think about it,
if that seems like a smart thing to do when the time comes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread David Steele
On 3/17/16 4:49 AM, Dean Rasheed wrote:

> On 16 March 2016 at 23:32, David Steele  wrote:
>
>>
>> I think in this case it comes down to a committer's judgement so I have
>> marked this "ready for committer" and passed the buck on to Álvaro.
> 
> So I was pretty much "meh" on this patch too, because I'm not
> convinced it actually saves much typing, if any.
> 
> However, I now realise that it introduces a backwards-compatibility
> breakage. Today it is possible to type
> 
> SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7 days');

It can also be broken as below and this is even scarier to me:

postgres=# SELECT * FROM generate_series('01-01-2000'::date,
'01-04-2000'::date, '7 days');
ERROR:  invalid input syntax for integer: "7 days"
LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7 days');

And only works when:

postgres=# SELECT * FROM generate_series('01-01-2000'::date,
'01-04-2000'::date, '7 days'::interval);
generate_series

 2000-01-01 00:00:00+00
(1 row)

One might argue that string constants for dates in actual code might be
rare but I think it's safe to say that string constants for intervals
are pretty common.  I also think it unlikely that they are all
explicitly cast.

I marked this "waiting for author" so Corey can respond.  I actually
tested with the v3 patch since the v4 patch seems to be broken with git
apply or patch:

$ patch -p1 < ../other/generate_series_date.v4.diff
patching file doc/src/sgml/func.sgml
Hunk #1 succeeded at 14787 (offset 87 lines).
Hunk #2 succeeded at 14871 (offset 87 lines).
patching file src/backend/utils/adt/date.c
patch:  malformed patch at line 163: diff --git
a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h

$ git apply -3 ../other/generate_series_date.v4.diff
fatal: corrupt patch at line 163

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread David G. Johnston
On Thu, Mar 17, 2016 at 7:57 AM, Corey Huinker 
wrote:

> On Thu, Mar 17, 2016 at 10:00 AM, David Steele 
> wrote:
>
>> On 3/17/16 4:49 AM, Dean Rasheed wrote:
>>
>> > On 16 March 2016 at 23:32, David Steele  wrote:
>> >
>> >>
>> >> I think in this case it comes down to a committer's judgement so I have
>> >> marked this "ready for committer" and passed the buck on to Álvaro.
>> >
>> > So I was pretty much "meh" on this patch too, because I'm not
>> > convinced it actually saves much typing, if any.
>> >
>> > However, I now realise that it introduces a backwards-compatibility
>> > breakage. Today it is possible to type
>> >
>> > SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7
>> days');
>>
>> It can also be broken as below and this is even scarier to me:
>>
>
Above and below are the same query​...


>
>> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
>> '01-04-2000'::date, '7 days');
>> ERROR:  invalid input syntax for integer: "7 days"
>> LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7 days');
>>
>> And only works when:
>>
>> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
>> '01-04-2000'::date, '7 days'::interval);
>> generate_series
>> 
>>  2000-01-01 00:00:00+00
>> (1 row)
>> --
>> -David
>> da...@pgmasters.net
>>
>
> Not sure what's wrong with the patch, but I get a clean one to you pending
> the outcome of the design discussion. v4 just ripped out the infinity
> tests, so v3 is valid for the issue you found..
>
> So the function clobbers the situation where the user meant a timestamp
> range but instead gave two dates, and meant an interval but gave a string.
> I'm curious how generate_series() for numeric didn't have the same issue
> with floats.
>
>
​The numeric forms use anyelement for all three arguments but the timestamp
version uses an explicit interval for the third.​


I see two ways around this:
>
> 1. Drop the step parameter entirely. My own use cases only ever require
> the step values 1 and -1, and which one the user wants can be inferred from
> (start < end). This would still change the output type where a person
> wanted timestamps, but instead input two dates.
>

​Undesirable.​

2. Rename the function date_series() or generate_series_date()
>
> I still think this is an important function because at the last several
> places I've worked, I've found myself manufacturing a query where some
> event data is likely to have date gaps, but we want to see the date gaps or
> at least have the 0 values contribute to a later average aggregate.
>
>
​I'd call it "generate_dates(...)" and be done with it.

We would then have:

generate_series()
generate_subscripts()
generate_dates()

David J.
​


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Tom Lane
Robert Haas  writes:
> On Thu, Mar 17, 2016 at 12:59 PM, Tom Lane  wrote:
>> One idea that might be worth considering is to define the function
>> as generate_series(date,date,interval) returns timestamp (without
>> time zone).  The point here would be only to move the behavior for
>> date inputs as far as getting timestamp without tz rather than
>> timestamp with tz; which would at least save some timezone rotations
>> in typical use, as well as rather debatable semantics.

> That's a fairly tenuous benefit, though, and a substantially different
> patch.

Well, some folks were already of the opinion that the patch's benefits
were tenuous ;-)

> I think it's time to give up here and move on.

Agreed, letting this go for now seems like the right move.  Maybe
someone will have a bright idea later.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread David Steele
On 3/17/16 11:30 AM, David G. Johnston wrote:
> On Thu, Mar 17, 2016 at 7:57 AM, Corey Huinker  >wrote:
> 
> On Thu, Mar 17, 2016 at 10:00 AM, David Steele  > wrote:
> 
> On 3/17/16 4:49 AM, Dean Rasheed wrote:
> 
> > On 16 March 2016 at 23:32, David Steele  > wrote:
> >
> >>
> >> I think in this case it comes down to a committer's judgement so I 
> have
> >> marked this "ready for committer" and passed the buck on to Álvaro.
> >
> > So I was pretty much "meh" on this patch too, because I'm not
> > convinced it actually saves much typing, if any.
> >
> > However, I now realise that it introduces a backwards-compatibility
> > breakage. Today it is possible to type
> >
> > SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7 
> days');
> 
> It can also be broken as below and this is even scarier to me:
> 
> 
> Above and below are the same query​...

Not sure I agree.  My point was that even if developers were pretty
careful with their casting (or are using two actual dates) then there's
still possibility for breakage.

> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000'::date, '7 days');
> ERROR:  invalid input syntax for integer: "7 days"
> LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7
> days');
> <...>
> 
> I see two ways around this: 
> 
> 1. Drop the step parameter entirely. My own use cases only ever
> require the step values 1 and -1, and which one the user wants can
> be inferred from (start < end). This would still change the output
> type where a person wanted timestamps, but instead input two dates.
> 
> ​Undesirable.​

Very undesirable.  Week intervals are a very valid use case and I don't
like the automagic interval idea.

> 
> 2. Rename the function date_series() or generate_series_date()
> 
> I still think this is an important function because at the last
> several places I've worked, I've found myself manufacturing a query
> where some event data is likely to have date gaps, but we want to
> see the date gaps or at least have the 0 values contribute to a
> later average aggregate.
> 
> 
> ​I'd call it "generate_dates(...)" and be done with it.
> 
> We would then have:
> 
> generate_series()
> generate_subscripts()
> generate_dates()

To me this completely negates the idea of this "just working" which is
why it got a +1 from me in the first place.  If I have to remember to
use a different function name then I'd prefer to just cast on the
timestamp version of generate_series().

Sorry, but this is now -1 from me, at least for this commitfest.  While
I like the idea I think it is far too late to be redesigning such a
minor feature.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Tom Lane
David Steele  writes:
> On 3/17/16 11:55 AM, David G. Johnston wrote:
>> With the first argument casted to date it doesn't matter whether you
>> cast the second argument as the pseudo-type anyelement will take its
>> value from the first argument and force the second to date.

> Ah, I see.

FWIW, there isn't any "anyelement" involved here, AFAICT.  The set
of functions we have today is:

regression=# \df generate*
 List of 
functions
   Schema   |Name | Result data type  | 
   Argument data types |  Type  
+-+---++
 pg_catalog | generate_series | SETOF bigint  | bigint, 
bigint | normal
 pg_catalog | generate_series | SETOF bigint  | bigint, 
bigint, bigint | normal
 pg_catalog | generate_series | SETOF integer | 
integer, integer   | normal
 pg_catalog | generate_series | SETOF integer | 
integer, integer, integer  | normal
 pg_catalog | generate_series | SETOF numeric | 
numeric, numeric   | normal
 pg_catalog | generate_series | SETOF numeric | 
numeric, numeric, numeric  | normal
 pg_catalog | generate_series | SETOF timestamp with time zone| 
timestamp with time zone, timestamp with time zone, interval   | normal
 pg_catalog | generate_series | SETOF timestamp without time zone | 
timestamp without time zone, timestamp without time zone, interval | normal
 pg_catalog | generate_subscripts | SETOF integer | 
anyarray, integer  | normal
 pg_catalog | generate_subscripts | SETOF integer | 
anyarray, integer, boolean | normal
(10 rows)

Now, generate_subscripts is a totally different function that *should*
have a separate name; it does not take a couple of endpoints.  That's
not much of an argument for inventing different names for some of
the functions that do work with a pair of endpoints.

The real problem is that if we invent generate_series(date,date,integer)
then, given the problem "generate_series(date,unknown,unknown)"
the parser's ambiguous-function rules[1] will resolve that as
generate_series(date,date,integer), on the grounds that that provides
more exact type matches (i.e. 1) than any of the other alternatives.
Previously, the same call would have been resolved as
generate_series(timestamptz,timestamptz,interval) on the grounds that
that's reachable by casting and timestamptz is a preferred type.
So if you had written such a call with an interval literal and didn't
bother to cast the literal, your code would break.

We could avoid that problem if the new function were defined as
generate_series(date,date,interval), but then I'm not certain
what the code ought to do if the given interval is not a multiple
of a day.

One idea that might be worth considering is to define the function
as generate_series(date,date,interval) returns timestamp (without
time zone).  The point here would be only to move the behavior for
date inputs as far as getting timestamp without tz rather than
timestamp with tz; which would at least save some timezone rotations
in typical use, as well as rather debatable semantics.  (The fact
that timestamptz is the preferred type in this hierarchy isn't
really doing us any favors here.)

regards, tom lane

[1] http://www.postgresql.org/docs/devel/static/typeconv-func.html


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread David Steele
On 3/17/16 1:17 PM, Robert Haas wrote:
> On Thu, Mar 17, 2016 at 12:59 PM, Tom Lane  wrote:
>> One idea that might be worth considering is to define the function
>> as generate_series(date,date,interval) returns timestamp (without
>> time zone).  The point here would be only to move the behavior for
>> date inputs as far as getting timestamp without tz rather than
>> timestamp with tz; which would at least save some timezone rotations
>> in typical use, as well as rather debatable semantics.  (The fact
>> that timestamptz is the preferred type in this hierarchy isn't
>> really doing us any favors here.)
> 
> That's a fairly tenuous benefit, though, and a substantially different
> patch.  I think it's time to give up here and move on.  We can revisit
> this for another release after we've had more time to think about it,
> if that seems like a smart thing to do when the time comes.

This has been marked "returned with feedback".

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Corey Huinker
On Thu, Mar 17, 2016 at 11:30 AM, David G. Johnston <
david.g.johns...@gmail.com> wrote:

> ​I'd call it "generate_dates(...)" and be done with it.
>

Sold. Hope to have a revised patch for you today or tomorrow.


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-19 Thread Corey Huinker
On Thu, Mar 17, 2016 at 10:00 AM, David Steele  wrote:

> On 3/17/16 4:49 AM, Dean Rasheed wrote:
>
> > On 16 March 2016 at 23:32, David Steele  wrote:
> >
> >>
> >> I think in this case it comes down to a committer's judgement so I have
> >> marked this "ready for committer" and passed the buck on to Álvaro.
> >
> > So I was pretty much "meh" on this patch too, because I'm not
> > convinced it actually saves much typing, if any.
> >
> > However, I now realise that it introduces a backwards-compatibility
> > breakage. Today it is possible to type
> >
> > SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7
> days');
>
> It can also be broken as below and this is even scarier to me:
>
> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000'::date, '7 days');
> ERROR:  invalid input syntax for integer: "7 days"
> LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7 days');
>
> And only works when:
>
> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000'::date, '7 days'::interval);
> generate_series
> 
>  2000-01-01 00:00:00+00
> (1 row)
>
> One might argue that string constants for dates in actual code might be
> rare but I think it's safe to say that string constants for intervals
> are pretty common.  I also think it unlikely that they are all
> explicitly cast.
>
> I marked this "waiting for author" so Corey can respond.  I actually
> tested with the v3 patch since the v4 patch seems to be broken with git
> apply or patch:
>
> $ patch -p1 < ../other/generate_series_date.v4.diff
> patching file doc/src/sgml/func.sgml
> Hunk #1 succeeded at 14787 (offset 87 lines).
> Hunk #2 succeeded at 14871 (offset 87 lines).
> patching file src/backend/utils/adt/date.c
> patch:  malformed patch at line 163: diff --git
> a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
>
> $ git apply -3 ../other/generate_series_date.v4.diff
> fatal: corrupt patch at line 163
>
> --
> -David
> da...@pgmasters.net
>

Not sure what's wrong with the patch, but I get a clean one to you pending
the outcome of the design discussion. v4 just ripped out the infinity
tests, so v3 is valid for the issue you found..

So the function clobbers the situation where the user meant a timestamp
range but instead gave two dates, and meant an interval but gave a string.
I'm curious how generate_series() for numeric didn't have the same issue
with floats.

I see two ways around this:

1. Drop the step parameter entirely. My own use cases only ever require the
step values 1 and -1, and which one the user wants can be inferred from
(start < end). This would still change the output type where a person
wanted timestamps, but instead input two dates.
2. Rename the function date_series() or generate_series_date()

I still think this is an important function because at the last several
places I've worked, I've found myself manufacturing a query where some
event data is likely to have date gaps, but we want to see the date gaps or
at least have the 0 values contribute to a later average aggregate.

Like this:

select d.dt, sum(s.v1), sum(s2.v2), sum(t.v1), sum(t2.v2)
from   date_series_function(input1,input2) as d(dt),
   some_dimensional_characteristic c
left join sparse_table s on s.event_date = d.dt and s.c1 = c.cval
left join sparse_table s2 on s2.event_date = d.dt and s2.c2 = c.cval
left join other_sparse t on t.event_date = d.dt and t.c1 = c.cval
left join other_sparse t2 on t2.event_date = d.dt and t2.c2 = c.cval
group by d.dt
order by d.dt

This gets cumbersome when you have to cast every usage of your date column.

Furthermore, this is 90%+ of my usage of generate_series(), the remaining
10% being the integer case to populate sample data for tests.

Any thoughts on how to proceed?


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-18 Thread Tom Lane
David Steele  writes:
> On 3/17/16 11:30 AM, David G. Johnston wrote:
>> ​I'd call it "generate_dates(...)" and be done with it.
>> We would then have:
>> generate_series()
>> generate_subscripts()
>> generate_dates()

> To me this completely negates the idea of this "just working" which is
> why it got a +1 from me in the first place.  If I have to remember to
> use a different function name then I'd prefer to just cast on the
> timestamp version of generate_series().

Yeah, this point greatly weakens the desirability of this function IMO.
I've also gone from "don't care" to "-1".

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

2016-03-18 Thread David G. Johnston
On Thu, Mar 17, 2016 at 8:41 AM, David Steele  wrote:

> On 3/17/16 11:30 AM, David G. Johnston wrote:
> > On Thu, Mar 17, 2016 at 7:57 AM, Corey Huinker  > >wrote:
> >
> > On Thu, Mar 17, 2016 at 10:00 AM, David Steele  > > wrote:
> >
> > On 3/17/16 4:49 AM, Dean Rasheed wrote:
> >
> > > On 16 March 2016 at 23:32, David Steele  > wrote:
> > >
> > >>
> > >> I think in this case it comes down to a committer's judgement
> so I have
> > >> marked this "ready for committer" and passed the buck on to
> Álvaro.
> > >
> > > So I was pretty much "meh" on this patch too, because I'm not
> > > convinced it actually saves much typing, if any.
> > >
> > > However, I now realise that it introduces a
> backwards-compatibility
> > > breakage. Today it is possible to type
> > >
> > > SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000', '7 days');
> >
> > It can also be broken as below and this is even scarier to me:
> >
> >
> > Above and below are the same query​...
>
> Not sure I agree.  My point was that even if developers were pretty
> careful with their casting (or are using two actual dates) then there's
> still possibility for breakage.
>

With the first argument casted to date it doesn't matter whether you cast
the second argument as the pseudo-type anyelement will take its value from
the first argument and force the second to date.

The main problem is that the system tries to cast unknown to integer based
upon finding gs(date, date, integer) and fails without ever considering
that gs(ts, ts, interval) would succeed.


> > postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> > '01-04-2000'::date, '7 days');
> > ERROR:  invalid input syntax for integer: "7 days"
> > LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7
> > days');
> > <...>
> >
> > I see two ways around this:
> >
> > 1. Drop the step parameter entirely. My own use cases only ever
> > require the step values 1 and -1, and which one the user wants can
> > be inferred from (start < end). This would still change the output
> > type where a person wanted timestamps, but instead input two dates.
> >
> > ​Undesirable.​
>
> Very undesirable.  Week intervals are a very valid use case and I don't
> like the automagic interval idea.
>
> >
> > 2. Rename the function date_series() or generate_series_date()
> >
> > I still think this is an important function because at the last
> > several places I've worked, I've found myself manufacturing a query
> > where some event data is likely to have date gaps, but we want to
> > see the date gaps or at least have the 0 values contribute to a
> > later average aggregate.
> >
> >
> > ​I'd call it "generate_dates(...)" and be done with it.
> >
> > We would then have:
> >
> > generate_series()
> > generate_subscripts()
> > generate_dates()
>
> To me this completely negates the idea of this "just working" which is
> why it got a +1 from me in the first place.  If I have to remember to
> use a different function name then I'd prefer to just cast on the
> timestamp version of generate_series().
>
>
​So let the user decide whether to trade-off learning a new function name
but getting dates instead of timestamps or going through the hassle of
casting.​

For me, it would have been a nice bonus if the generate_series() could be
used directly but I feel that the desired functionality is desirable and
the name itself is of only minor consideration.

I can see that newbies might ponder why two functions exist but they should
understand "backward compatibility".

I suspect that most people will simply think: "use generate_series for
numbers and use generate_dates for, well, dates".  The ones left out in the
cold are those wondering why they use "generate_series" for timestamp
series with a finer precision than date.  I'd be willing to offer a
"generate_timestamps" to placate them.  And might as well toss in
"generate_numbers" for completeness - though now I likely doom the
proposal...so I'll stick with just add generate_dates so the behavior is
possible without superfluous casting or the need to write a custom
function.  Dates are too common a unit of measure to leave working with
them this cumbersome.

David J.


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-03-04 Thread David Steele
On 2/21/16 2:24 PM, Vik Fearing wrote:
> On 02/21/2016 07:56 PM, Corey Huinker wrote:
>>
>> Other than that, the only difference is the ::date part. Is it
>> really worth adding extra code just for that? I would say not.
>>
>>
>> I would argue it belongs for the sake of completeness. 
> 
> So would I.
> 
> +1 for adding this missing function.

+1. FWIW, a sample query I wrote for a customer yesterday would have
looked nicer with this function. Here's how the generate_series looked:

generate_series('2016-03-01'::date, '2016-03-31'::date, interval '1
day')::date

But it would have been cleaner to write:

generate_series('2016-03-01'::date, '2016-03-31'::date)

More importantly, though, I don't like that the timestamp version of the
function happily takes date parameters but returns timestamps. I feel
this could lead to some subtle bugs for the unwary.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-21 Thread Vik Fearing
On 02/21/2016 07:56 PM, Corey Huinker wrote:
> 
> Other than that, the only difference is the ::date part. Is it
> really worth adding extra code just for that? I would say not.
> 
> 
> I would argue it belongs for the sake of completeness. 

So would I.

+1 for adding this missing function.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-21 Thread Corey Huinker
>
>
> [step] is in days, but is not documented as such.
>
>
It is in days, and is not documented as such, but since a day is the
smallest unit of time for a date, I felt there was no other interpretation.



> My understanding is you want to replace this
>
> SELECT d.dt::date as dt
> FROM generate_series('2015-01-01'::date,
>  '2016-01-04'::date,
>  interval '1 day') AS d(dt);
>
>
> with this
>
> SELECT d.dt
> FROM generate_series('2015-01-01'::date,
>  '2016-01-04'::date,
>  7) as d(dt);
>
>

I'd also like to be able to join the values of d.dt without typecasting
them.
To me it's as awkward as (select 1::double + 1::double)::integer


>
> Personally, I think writing  INTERVAL '7 days' to be clearer than just
> typing 7.
>

Well, nearly all my use cases involve the step being 1 (and thus omitted)
or -1.

Maybe this example will appeal to you

SELECT d.birth_date, COUNT(r.kiddo_name)
FROM generate_series('2016-01-01'::date,'2016-01-10'::date) as d(birth_date)
LEFT OUTER JOIN birth_records r ON r.birth_date = d.birth_date
GROUP BY 1
ORDER BY 1;



> Other than that, the only difference is the ::date part. Is it really
> worth adding extra code just for that? I would say not.
>

I would argue it belongs for the sake of completeness.
We added generate_series for numerics when generate_series for floats
already existed.

No comments on the patch itself, which seems to do the job, so apologies to
> give this opinion on your work, I do hope it doesn't put you off further
> contributions.
>

Thanks. I appreciate that.


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-20 Thread Simon Riggs
On 2 February 2016 at 18:01, Corey Huinker  wrote:

> Doh, I left that comment to myself in there. :)
>
> The corresponding structs in timestamp.c and int.c have no comment, so
> suggestions of what should be there are welcome. In the interim I put in
> this:
>
> /* state for generate_series_date(date,date,[step]) */
>
>
> Extra linefeed after struct removed.
>
> Do you have any insight as to why the documentation test failed?
>
> In the mean time, here's the updated patch.
>

[step] is in days, but is not documented as such.

My understanding is you want to replace this

SELECT d.dt::date as dt
FROM generate_series('2015-01-01'::date,
 '2016-01-04'::date,
 interval '1 day') AS d(dt);


with this

SELECT d.dt
FROM generate_series('2015-01-01'::date,
 '2016-01-04'::date,
 7) as d(dt);


Personally, I think writing  INTERVAL '7 days' to be clearer than just
typing 7.

Other than that, the only difference is the ::date part. Is it really worth
adding extra code just for that? I would say not.

No comments on the patch itself, which seems to do the job, so apologies to
give this opinion on your work, I do hope it doesn't put you off further
contributions.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-02 Thread David Steele
On 2/2/16 1:01 PM, Corey Huinker wrote:
> Doh, I left that comment to myself in there. :)

If I had a dime for every time I've done that...

> The corresponding structs in timestamp.c and int.c have no comment, so
> suggestions of what should be there are welcome. In the interim I put in
> this:
> 
> /* state for generate_series_date(date,date,[step]) */

I think that's fine -- whoever commits it may feel otherwise.

> Do you have any insight as to why the documentation test failed?
> 
> In the mean time, here's the updated patch.

I didn't pass the docs on account of the wonky comment since I consider
code comments to be part of the docs.  The sgml docs build fine and look
good to me.

I'll retest and update the review accordingly.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-02 Thread Corey Huinker
Doh, I left that comment to myself in there. :)

The corresponding structs in timestamp.c and int.c have no comment, so
suggestions of what should be there are welcome. In the interim I put in
this:

/* state for generate_series_date(date,date,[step]) */


Extra linefeed after struct removed.

Do you have any insight as to why the documentation test failed?

In the mean time, here's the updated patch.


On Tue, Feb 2, 2016 at 11:41 AM, David Steele  wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, failed
>
> Everything looks good except for two minor issues:
>
> 1) There should be an informative comment for this struct:
>
> +/* Corey BEGIN */
> +typedef struct
> +{
> +   DateADT current;
> +   DateADT stop;
> +   int32   step;
> +} generate_series_date_fctx;
>
> 2) There's an extra linefeed after the struct.  Needed?
>
> Regards,
> -David
>
> The new status of this patch is: Waiting on Author
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


v3.generate_series_date.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

2016-02-02 Thread Corey Huinker
>
>
> > Do you have any insight as to why the documentation test failed?
> >
> > In the mean time, here's the updated patch.
>
> I didn't pass the docs on account of the wonky comment since I consider
> code comments to be part of the docs.  The sgml docs build fine and look
> good to me.
>
>
Ah, understood. I rebuilt the docs thinking there was a make check failure
I missed, then viewed the html in links (I develop on an AWS box) and saw
nothing untoward.


> I'll retest and update the review accordingly.
>

Thanks!