Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PGInterval#getSeconds should not be double #748

Closed
marschall opened this issue Feb 12, 2017 · 6 comments
Closed

PGInterval#getSeconds should not be double #748

marschall opened this issue Feb 12, 2017 · 6 comments

Comments

@marschall
Copy link
Contributor

In Java 9 java.time.Duration introduces new methods that make it possible to convert it to and from PGInterval.

One obstacle that is currently blocking this is the seconds field and the associated getSeconds method which contains both the whole seconds and the microseconds. Unfortunately this is of type double which is not an exact data type but rather a floating point which has limited precision. In addition to introducing possible imprecision and data loss this also regularly confuses developers, see for example this article.

Changing the return type or even semantic of the getSeconds method would break existing clients so I assume this is a no-go.

My proposal would therefore be to introduce two new methods, one for the whole seconds part called getWholeSeconds or getSecondsPart and one for the microseconds called getMicros or getMicro with a backing field for each of those. The getSeconds method could be implemented in terms of these methods and maybe deprecated. The seconds field can be removed.

This will change the serialized form and cause people to no longer being able to load in old serialized PGInterval instances but I believe this can be tolerated. None of the PGobject subclasses define a serialVersionUID so things like 7aac95c already broke serialized form and I'm not aware of any complaints.

Since we already have to change the serialised form we should consider changing the months, days, hours, minutes and (new) seconds fields to type byte instead of int (years can go to 178000000 so that's not an option) while keeping the return and argument types of the methods. This would save a bit of memory. However this would either require normalization or input validation. Neither of which are currently done. Eg. currently you can call #setMonths(12000) which leaves the years field untouched. That would either have to change the years either by discarding or incrementing the current value or throwing an exception. Former seems highly problematic and the latter is a breaking change as well. So I would leave the field types as they currently are.

@davecramer
Copy link
Member

In general if your assumption that nobody is really using the serialized form then I see no problem changing this.

@davecramer
Copy link
Member

@marschall any interest in picking this up again ?

@marschall
Copy link
Contributor Author

I'm afraid I won't have time to work on this in the foreseeable future.

@davecramer
Copy link
Member

OK, I may take a crack at it.

@davecramer
Copy link
Member

So there are a number of things wrong with PGInterval
This is legal select '+2004 years -14 mons +20 days -15:57:12.1'::interval
and neither of the ISO 8601 formats are supported at all. 'P1Y2M3DT4H5M6S'

@davecramer
Copy link
Member

fixed in #1612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants