John Naylor <john.nay...@2ndquadrant.com> writes: > [ v3-datetrunc_interval.patch ]
A few thoughts: * In general, binning involves both an origin and a stride. When working with plain numbers it's almost always OK to set the origin to zero, but it's less clear to me whether that's all right for timestamps. Do we need another optional argument? Even if we don't, "zero" for tm_year is 1900, which is going to give results that surprise somebody. * I'm still not convinced that the code does the right thing for 1-based months or days. Shouldn't you need to subtract 1, then do the modulus, then add back 1? * Speaking of modulus, would it be clearer to express the calculations like timestamp -= timestamp % interval; (That's just a question, I'm not sure.) * Code doesn't look to have thought carefully about what to do with negative intervals, or BC timestamps. * The comment * Justify all lower timestamp units and throw an error if any * of the lower interval units are non-zero. doesn't seem to have a lot to do with what the code after it actually does. Also, you need explicit /* FALLTHRU */-type comments in that switch, or pickier buildfarm members will complain. * Seems like you could jam all the unit-related error checking into that switch's default: case, where it will cost nothing if the call is valid: switch (unit) { ... default: if (unit == 0) // complain about zero interval else // complain about interval with multiple components } * I'd use ERRCODE_INVALID_PARAMETER_VALUE for any case of disallowed contents of the interval. regards, tom lane