Re: Interval support for Postgres

Hi folks,

here's a patch that adds functionality to the PGInterval class. I'd be happy
to see this in upstream.

I'll enhance the unit tests if you want to apply this to upstream.

Best,
Oliver

On Thu, 21 Apr 2005, Oliver Siegmar wrote:

> here's a patch that adds functionality to the PGInterval class. I'd be happy
> to see this in upstream.

You cannot use regex code in this class because it must compile with 1.2
and 1.3 JDKs.

Kris Jurka

On Thursday 21 April 2005 20:32, Kris Jurka wrote:
> On Thu, 21 Apr 2005, Oliver Siegmar wrote:
> > here's a patch that adds functionality to the PGInterval class. I'd be
> > happy to see this in upstream.
>
> You cannot use regex code in this class because it must compile with 1.2
> and 1.3 JDKs.

Can it be placed in the JDBC3 package, then? This requires JDK 1.4 anyway,
right?

Oliver

On Thu, 21 Apr 2005, Oliver Siegmar wrote:

> On Thursday 21 April 2005 20:32, Kris Jurka wrote:
> > On Thu, 21 Apr 2005, Oliver Siegmar wrote:
> > > here's a patch that adds functionality to the PGInterval class. I'd be
> > > happy to see this in upstream.
> >
> > You cannot use regex code in this class because it must compile with 1.2
> > and 1.3 JDKs.
>
> Can it be placed in the JDBC3 package, then? This requires JDK 1.4 anyway,
> right?
>

It could, but I'd be against it. Having a different PGInterval behavior
for JDBC2 and JDBC3 builds would be rather odd. While regex may be
convenient here it certainly isn't essential. If you want to get this in
I'd recommend rewriting to do the parsing manually.

Kris Jurka

On Thursday 21 April 2005 21:43, Kris Jurka wrote:
> On Thu, 21 Apr 2005, Oliver Siegmar wrote:
> > On Thursday 21 April 2005 20:32, Kris Jurka wrote:
> > > On Thu, 21 Apr 2005, Oliver Siegmar wrote:
> > > > here's a patch that adds functionality to the PGInterval class. I'd
> > > > be happy to see this in upstream.
> > >
> > > You cannot use regex code in this class because it must compile with
> > > 1.2 and 1.3 JDKs.
> >
> > Can it be placed in the JDBC3 package, then? This requires JDK 1.4
> > anyway, right?
>
> It could, but I'd be against it. Having a different PGInterval behavior
> for JDBC2 and JDBC3 builds would be rather odd. While regex may be
> convenient here it certainly isn't essential. If you want to get this in
> I'd recommend rewriting to do the parsing manually.

Well, actually there is almost no implementation at all. Parsing strings in
Java in 2005 is a royal pain in the ass - doing that for 0.x% users suck even
more.

Let's assume I'd write a JDBC2 version with old-school string parsing - would
you apply it to upstream, or are there any other show stoppers?

Oliver

On Thu, 21 Apr 2005, Oliver Siegmar wrote:

> Well, actually there is almost no implementation at all. Parsing strings in
> Java in 2005 is a royal pain in the ass - doing that for 0.x% users suck even
> more.

Well, just be thankful the 8.0 release finally dropped JDK 1.1 support.

> Let's assume I'd write a JDBC2 version with old-school string parsing - would
> you apply it to upstream, or are there any other show stoppers?
>

We definitely want something better for PGInterval. I do have some
questions about the implementation you've got.

1) You have an "int milliseconds" field. PG supports microsecond
precision in intervals. Is there any reason not to go with a plain "float
seconds" field instead of splitting these up?

2) getValue() calls nf.format(Math.abs(milliseconds)), if milliseconds is
2 won't the NumberFormat(ter) turn that into .02 ?

3) In general the whole "before" API is confusing. Do you have any better
ideas on how to handle this?
a) Note getValue() returns the wrong value if hours == 0 and minutes < 0.
b) Doesn't clone() need to copy the before setting?
c) After a setValue() if someone does setYear() don't they need to
know the before setting (which is protected).

Kris Jurka

Kris Jurka wrote:

> 1) You have an "int milliseconds" field. PG supports microsecond
> precision in intervals. Is there any reason not to go with a plain "float
> seconds" field instead of splitting these up?

Can you represent all of 0.000000 .. 59.999999 exactly as floats?

-O

Hi Kris,

On Thursday 21 April 2005 23:00, Kris Jurka wrote:
> 1) You have an "int milliseconds" field. PG supports microsecond
> precision in intervals. Is there any reason not to go with a plain "float
> seconds" field instead of splitting these up?

Working with floating points is more difficult because of the requirements of
Calendar's roll() - but see the new patch, it uses double for seconds (and
microseconds).

> 2) getValue() calls nf.format(Math.abs(milliseconds)), if milliseconds is
> 2 won't the NumberFormat(ter) turn that into .02 ?

Right, fixed.

> 3) In general the whole "before" API is confusing. Do you have any better
> ideas on how to handle this?

Yes, done.

> a) Note getValue() returns the wrong value if hours == 0 and minutes < 0.

Right, fixed.

> b) Doesn't clone() need to copy the before setting?

No, because it was only used to initialize the values - "before" is dropped
anyway.

> c) After a setValue() if someone does setYear() don't they need to
> know the before setting (which is protected).

I thought these setters only for internal use (they all were protected). Now
they're public and "before" has gone.

Take a look at the new patch, I'm sure you'll find tons of things you don't
like... ;-)

Oliver

On Thursday 21 April 2005 17:47, Oliver Jowett wrote:
> Kris Jurka wrote:
> > 1) You have an "int milliseconds" field. PG supports microsecond
> > precision in intervals. Is there any reason not to go with a
> > plain "float seconds" field instead of splitting these up?
>
> Can you represent all of 0.000000 .. 59.999999 exactly as floats?

Seem like you more or less can -- up to 16.000001:

| $ cat QuickTest.java
| public class QuickTest {
| public static void main(String[] _) {
| System.out.println("16.000001f - 16.000000f = " +
| (16.000001f - 16.000000f));
|
| System.out.println("16.000002f - 16.000001f = " +
| (16.000002f - 16.000001f));
| }
| }
|
| $ javac QuickTest.java
|
| $ java -cp . QuickTest
| 16.000001f - 16.000000f = 1.9073486E-6
| 16.000002f - 16.000001f = 0.0

On Thursday 21 April 2005 23:00, Kris Jurka wrote:
> On Thu, 21 Apr 2005, Oliver Siegmar wrote:
> > Well, actually there is almost no implementation at all. Parsing strings
> > in Java in 2005 is a royal pain in the ass - doing that for 0.x% users
> > suck even more.
>
> Well, just be thankful the 8.0 release finally dropped JDK 1.1 support.

The applied patch contains all modifications from the last one
(pgjdbc_interval2.diff) as well as a replacement for the regular expression
code.

The patch should be JDK 1.2 compatible, now.

Oliver

On Sat, 23 Apr 2005, Oliver Siegmar wrote:

> The applied patch contains all modifications from the last one
> (pgjdbc_interval2.diff) as well as a replacement for the regular expression
> code.
>

Here's my version of it. Changes:
- Always use noniso format for sending intervals to the backend.
This makes it possible to support a seconds field of 90. Also
don't need to worry about signs of fields.
- Calendar.getTimeInMillis is protected before 1.4
- Added db access to the test case

My one remaining question is about the roll function. What is the purpose
of roll, don't we want add? Ignoring carryover seems like a mistake.
Also in the roll method the milliseconds calculation should round the
value so 0.6006 seconds becomes 601 milliseconds.

Kris Jurka

On Friday 29 April 2005 10:01, Kris Jurka wrote:
> On Sat, 23 Apr 2005, Oliver Siegmar wrote:
> > The applied patch contains all modifications from the last one
> > (pgjdbc_interval2.diff) as well as a replacement for the regular
> > expression code.
>
> Here's my version of it. Changes:
> - Always use noniso format for sending intervals to the backend.
> This makes it possible to support a seconds field of 90. Also
> don't need to worry about signs of fields.

Sounds reasonable. But I'm not sure if we should support "oversized" values -
I see no requirement for it, and it adds a lot of possible trouble.

Anyway...I have used the + concatination (in getValue()) knowingly, because
JDK < 5.0 compiles that to StringBuffer and JDK 5.0 to StringBuilder
automatically. The usage of StringBuffer and StringBuilder makes only sense
if the string isn't built as a one-liner.

> My one remaining question is about the roll function. What is the purpose
> of roll, don't we want add?

a) because the database doesn't return "oversized" values - so it doesn't
matter using add() or roll()

b) because add() is a beast. Consider the following simple example:

---------------------------------------------------------------------------
01: Calendar cal1 = Calendar.getInstance();
02: Calendar cal2 = (Calendar)cal1.clone();
03:
04: System.out.println(cal1.equals(cal2)); // returns true, of course
05:
06: cal1.add(Calendar.MONTH, -4);
07: cal1.add(Calendar.DAY_OF_WEEK_IN_MONTH, 20);
08:
09: cal1.add(Calendar.MONTH, 4);
10: cal1.add(Calendar.DAY_OF_WEEK_IN_MONTH, -20);
11:
12: System.out.println(cal1.equals(cal2)); // returns false
---------------------------------------------------------------------------

But if you twist line 9 and 10 - everything is ok. If you use add() you have
to use the inverse order if you want to "roll" backwards, again. I see no way
to get a PGInterval.add() method acting symmetric.

> Ignoring carryover seems like a mistake.

Not if all values are "in range". The method name roll() won't be
misunderstood, I think.

> Also in the roll method the milliseconds calculation should round the
> value so 0.6006 seconds becomes 601 milliseconds.

Why? When is the current implementation doing wrong?

Oliver

On Fri, 29 Apr 2005, Oliver Siegmar wrote:

> On Friday 29 April 2005 10:01, Kris Jurka wrote:
> > On Sat, 23 Apr 2005, Oliver Siegmar wrote:
> > > The applied patch contains all modifications from the last one
> > > (pgjdbc_interval2.diff) as well as a replacement for the regular
> > > expression code.
> >
> > Here's my version of it. Changes:
> > - Always use noniso format for sending intervals to the backend.
> > This makes it possible to support a seconds field of 90. Also
> > don't need to worry about signs of fields.
>
> Sounds reasonable. But I'm not sure if we should support "oversized" values -
> I see no requirement for it, and it adds a lot of possible trouble.

Well the constructor and setXXX methods allow it. I'm not saying we need
to go out of our way to make all excessive values possible, but if we can
allow say 90 seconds with no other harm, why not? If we're going to
prevent it, we should prevent it at set time, not at getValue time.

> > My one remaining question is about the roll function. What is the purpose
> > of roll, don't we want add?
>
> a) because the database doesn't return "oversized" values - so it doesn't
> matter using add() or roll()

Surely it does, consider a calendar 2001-01-01 00:00:45 and an interval of
20 seconds. roll gives 2001-01-01 00:00:05 while add gives the
correct 2001-01-01 00:01:05

> > Also in the roll method the milliseconds calculation should round the
> > value so 0.6006 seconds becomes 601 milliseconds.
>
> Why? When is the current implementation doing wrong?
>

final int milliseconds = microseconds / 1000 - seconds * 1000;

Shouldn't this be "microseconds + 500 / 1000" (although it does need to
account for the sign.

Kris Jurka

Hi Kris,

On Friday 29 April 2005 12:35, Kris Jurka wrote:
> > > My one remaining question is about the roll function. What is the
> > > purpose of roll, don't we want add?
> >
> > a) because the database doesn't return "oversized" values - so it doesn't
> > matter using add() or roll()
>
> Surely it does, consider a calendar 2001-01-01 00:00:45 and an interval of
> 20 seconds. roll gives 2001-01-01 00:00:05 while add gives the
> correct 2001-01-01 00:01:05

Oh...I must have goofed. You're right - I tried to implement a working add()
version.

In the initial implementation of add() (a never sent implementation) I was too
demanding. It is just not possible to "roll" every date forth and back and
get the initial date, of course. Just think of Mar 31 plus one month and then
one month back...that cannot be Mar 31 and that's okay. This has to be
considered when testing - not any starting date can be used to roll forth and
back.

> Shouldn't this be "microseconds + 500 / 1000" (although it does need to
> account for the sign.

Yeah, good point - applied that, too.

Give pgjdbc_interval5.diff a new try ;-)

Oliver

On Fri, 29 Apr 2005, Oliver Siegmar wrote:

> Give pgjdbc_interval5.diff a new try ;-)
>

Applied. Thanks.

Kris Jurka

On Friday 29 April 2005 22:42, Kris Jurka wrote:
> On Fri, 29 Apr 2005, Oliver Siegmar wrote:
> > Give pgjdbc_interval5.diff a new try ;-)
>
> Applied. Thanks.

Thanks. Could you also add it to the 8.0 branch? That'd be great!

Oliver

On Sat, 30 Apr 2005, Oliver Siegmar wrote:

> > On Fri, 29 Apr 2005, Oliver Siegmar wrote:
> > > Give pgjdbc_interval5.diff a new try ;-)
>
> Thanks. Could you also add it to the 8.0 branch? That'd be great!
>

That's not really our policy, new features go into head, bug fixes only in
released branches. This isn't a terribly invasive patch, but it does make
some behavioral differences (notably an existing parser of getValue()
would only expect ISO format, not pg format). I don't think a convincing
case can be made for needing this in 8.0, but you are free to try and
convince other users and developers.

Kris Jurka

On Saturday 30 April 2005 10:25, Kris Jurka wrote:
> On Sat, 30 Apr 2005, Oliver Siegmar wrote:
> > > On Fri, 29 Apr 2005, Oliver Siegmar wrote:
> > > > Give pgjdbc_interval5.diff a new try ;-)
> >
> > Thanks. Could you also add it to the 8.0 branch? That'd be great!
>
> That's not really our policy, new features go into head, bug fixes only in
> released branches. This isn't a terribly invasive patch, but it does make
> some behavioral differences (notably an existing parser of getValue()
> would only expect ISO format, not pg format). I don't think a convincing
> case can be made for needing this in 8.0, but you are free to try and
> convince other users and developers.

I understand. Well, it depends a lot on when 8.1 driver will be released as a
stable version (or how stable prereleases are for productional use). If a 8.1
JDBC-Driver release is planned for mid-2005, I'm okay. Otherwise it would be
imho a too long time to work with prereleases to get interval support.

...just my two cents...

Oliver

On Sat, 30 Apr 2005, Oliver Siegmar wrote:

> I understand. Well, it depends a lot on when 8.1 driver will be released as a
> stable version (or how stable prereleases are for productional use). If a 8.1
> JDBC-Driver release is planned for mid-2005, I'm okay. Otherwise it would be
> imho a too long time to work with prereleases to get interval support.

The JDBC driver follows the server release schedule, which is roughly
yearly. 8.0 was released 1/2005 so a good guess at 8.1 is 1/2006. As you
well know it is an open source project, so people who need just interval
support in 8.0 can backpatch just that code. I think I can recall only
one other person in the past two years or so asking about extracting
interval fields in java, so I don't see this as a must have feature.

Kris Jurka