Fractional seconds in intervals truncated

[keithlayne]

Postgraphile is truncating the seconds in intervals with fractional seconds.

There was no test before, so I'm not sure exactly what has changed. Looking at the bendrucker/postgres-interval I see that the milliseconds field on intervals has been there since before the major version change. I suspect that using intervals in postgraphile has always behaved this way, so possibly this is a non-breaking change, but I can't confirmm easily.

I'm not sure whether this will be considered a feature or a fix, so please edit the title as you see fit. I plan to update this PR to include the milliseconds field in Interval. Side note, I also want to make the interval fields non-nullable - is that okay/not okay/should be separate?

[keithlayne]

Sorry, this PR is really really noisy. I didn't squash so that hopefully it would be a little easier to see the changes. I accepted the snapshot updates so you could see the diffs here. I can squash if you want but I know you like to fix things up how you like it, so whatever works.

[keithlayne]

BTW my rationale for this being non-breaking:

I think before the seconds were effectively Int. In JS an int and float are both floats, and making the fields non-nullable on queries should be backwards-compatible.

Hmmm. Didn't think about the inputs though, if people are passing non-ints then graphql validation could break I guess. I didn't actually check to see if fractional seconds actually were persisted correctly or not. Not sure how to test that easily, probably put seconds: 1.5 into a mutation and look at the snapshot.

I guess a workaround would be to make the seconds input a float until a breaking change, with a note. I'm just talking, I'm sure you'll sort it out.

[keithlayne]

Note to self: look into applying justify_interval to inputs. That would seem to be the least surprising thing to do. See note right below https://www.postgresql.org/docs/11/datatype-datetime.html#DATATYPE-INTERVAL-INPUT-EXAMPLES for context.

[keithlayne]

In retrospect, probably overdid it with changing the fixtures, I think it would take a much smaller change to test this.

Also, with the question of whether these are breaking changes (sorry, there are like 3 different things in this PR) I find myself kinda wishing this were either an independent plugin or somehow configurable, so that this change could be put behind a feature flag or equivalent.

[keithlayne]

Fun stuff:

select interval '1.2 years 3.4 months 5.6 weeks 7.8 days 9.1 hours 2.3 minutes 4 seconds 6.7milliseconds';
┌─────────────────────────────────────┐
│              interval               │
├─────────────────────────────────────┤
│ 1 year 5 mons 58 days 33:08:22.0067 │
└─────────────────────────────────────┘
select justify_interval('1.2 years 3.4 months 5.6 weeks 7.8 days 9.1 hours 2.3 minutes 4 seconds 6.7milliseconds');
┌─────────────────────────────────────┐
│          justify_interval           │
├─────────────────────────────────────┤
│ 1 year 6 mons 29 days 09:08:22.0067 │
└─────────────────────────────────────┘

BUT pg will give you an error if you combine non-integer seconds with milliseconds (which seems pretty reasonable):

select interval '4s 6.7ms';
┌───────────────┐
│   interval    │
├───────────────┤
│ 00:00:04.0067 │
└───────────────┘
select interval '4.5s 6.7ms';
ERROR:  22007: invalid input syntax for type interval: "4.5s 6.7ms"

[benjie]

Thanks for spotting and attempting to fix this issue. Unfortunately changing seconds from Float to Int is a breaking change; though people consuming GraphQL from JavaScript/TypeScript may not notice it, other strongly typed languages likely will - this is why GraphQL differentiates these types, rather than just having GraphQLNumber. Further, our input syntax supports milliseconds on the seconds field (I think?) so this would again be breaking for existing queries. (And for operations with variables declared with $seconds: Float, the operations would now be invalid.)

Rather than taking the approach you have by adding the milliseconds field, we should correct the resolution for seconds to also include the milliseconds/etc such that it complies with its own documentation. This should be feasible by adding a resolve method to the definition of seconds when used as an output:

      const makeIntervalFields = (input = false) => {
        return {
          seconds: {
            description:
              "A quantity of seconds. This is the only non-integer field, as all the other fields will dump their overflow into a smaller unit of time. Intervals don’t have a smaller unit than seconds.",
            type: GraphQLFloat,

            // For the output type, override the seconds resolver
			...(input ? {} : {
              resolve(interval) { return interval.seconds + interval.milliseconds * 1e-3 }
            }),

          },

NOTE: PostgreSQL supports timestamps to nanosecond precision; assuming we don't overflow the number of seconds significantly, it's possible to represent this maximum value 59.999999999 precisely in IEEE754 floating point numbers, so this should be safe to continue to use Float rather than String.

NOTE: I have not checked what postgres-interval returns, but you should be sure to check that the correct number of nanoseconds is returned. Try using a more troublesome value in the tests, such as 59.876543210 seconds.

This change should significantly reduce the size of the diff in this PR also - i.e. there should be no schema changes 👍 If you want to change the nullability of these fields, please do so in a separate PR.

[benjie]

Please don't use justify_interval; it changes the meaning of the interval. Note that the first is correct, but the second (using justify_interval) is incorrectly 2 days later!

[test] # select '2019-02-01T00:00:00Z'::timestamptz + interval '1 month -1 hour';
┌────────────────────────┐
│        ?column?        │
├────────────────────────┤
│ 2019-02-28 23:00:00+00 │
└────────────────────────┘
(1 row)

Time: 0.216 ms
[test] # select '2019-02-01T00:00:00Z'::timestamptz + justify_interval(interval '1 month -1 hour');
┌────────────────────────┐
│        ?column?        │
├────────────────────────┤
│ 2019-03-02 23:00:00+00 │
└────────────────────────┘
(1 row)

Time: 7.468 ms

Even without the negative values you can see the results differ:

[test] # select '2019-02-01T00:00:00Z'::timestamptz + interval '30 days';
┌────────────────────────┐
│        ?column?        │
├────────────────────────┤
│ 2019-03-03 00:00:00+00 │
└────────────────────────┘
(1 row)

Time: 0.234 ms
[test] # select '2019-02-01T00:00:00Z'::timestamptz + justify_interval(interval '30 days');
┌────────────────────────┐
│        ?column?        │
├────────────────────────┤
│ 2019-03-01 00:00:00+00 │
└────────────────────────┘
(1 row)

Time: 0.245 ms

[benjie]

Use more troublesome values here; PostgreSQL supports nanosecond precision:

Suggested change

	'1 year 2 months 3 days 4 hours 5 minutes 6.5 seconds',
	ARRAY['1 year 2 months 3 days 4 hours 5 minutes 6 seconds', '1 year 1 months 1 days 1 hours 1 minutes 1.234 seconds', '123.456 milliseconds']::interval[],
	'1 year 2 months 3 days 4 hours 5 minutes 59.876543210 seconds',
	ARRAY['1 year 2 months 3 days 4 hours 5 minutes 6 seconds', '1 year 1 months 1 days 1 hours 1 minutes 1.234567890 seconds', '123.456789012 milliseconds']::interval[],

[benjie]

Thanks so much for the PR and discussion @keithlayne; I decided to fix this in another way in the end but I really appreciate your work 🙏