Gurjeet Singh wrote on 30.01.2023 08:18:On Sat, Nov 12, 2022 at 10:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, what it shouldn't be is part of this patch. It's worth
pushing it separately to have a record of that decision. I've
now done that, so you'll need to rebase to remove that delta.
I looked over the v5 patch very briefly, and have two main
complaints:
* There's no documentation additions. You can't add a user-visible
function without adding an appropriate entry to func.sgml.
* I'm pretty unimpressed with the whole truncate-to-interval thing
and would recommend you drop it. I don't think it's adding much
useful functionality beyond what we can already do with the existing
date_trunc variants; and the definition seems excessively messy
(too many restrictions and special cases).
Please see attached v6 of the patch.
The changes since v5 are:
.) Rebased and resolved conflicts caused by commit 533e02e92.
.) Removed code and tests related to new date_trunc() functions, as
suggested by Tom.
.) Added 3 more variants to accompany with date_add(tstz, interval, zone). date_add(tstz, interval) date_subtract(tstz, interval) date_subtract(tstz, interval, zone)
.) Eliminate duplication of code; use common function to implement
generate_series_timestamptz[_at_zone]() functions.
.) Fixed bug where in one of the new code paths,
generate_series_timestamptz_with_zone(), did not perform
TIMESTAMP_NOT_FINITE() check.
.) Replaced some DirectFunctionCall?() with direct calls to the
relevant *_internal() function; should be better for performance.
.) Added documentation all 5 functions (2 date_add(), 2
date_subtract(), 1 overloaded version of generate_series()).
Other work distracted me from this patch.
I looked at your update v6 and it looks ok.
For me the date_trunc function is important and I still have some corner cases. Now I will continue working with data_trunc in a separate patch.
I'm not sure of the convention around authorship. But since this was
not an insignificant amount of work, would this patch be considered as
co-authored by Przemyslaw and myself? Should I add myself to Authors
field in the Commitfest app?
I see no obstacles for us to be co-authors.
Hi Przemyslaw, I started working on this patch based on Tom's review a few days
ago, since you hadn't responded in a while, and I presumed you're not
working on this anymore. I should've consulted with/notified you of my
intent before starting to work on it, to avoid duplication of work.
Sorry if this submission obviates any work you have in progress.
Please feel free to provide your feedback on the v6 of the patch.
I propose to get the approval of the current truncated version of the patch. As I wrote above, I will continue work on date_trunc later and as a separate patch.
--
Przemysław Sztoch | Mobile +48 509 99 00 66