Re: Patch implementing escaped functions timestampadd and

Hi,

I wrote the missing translations for escaped functions timestampadd and
timestampdiff. There are two patchs : one for the code+test and one for
the documentation.

Xavier Poinsard.

Xavier,

Thanks, any chance we could get a context diff instead of a plain diff ?

Also I looked at it briefly, can you change the if (SQL_TSI_DAY. ....
checks to check for "SQL_TSI" , before you even create the
stringbuffer in sqltimestampdiff, and sqltimestampadd

Ideally it would be good to verify all of the functions in the test .

Dave

Dave
On 22-Mar-06, at 3:38 AM, Xavier Poinsard wrote:

> Hi,
>
> I wrote the missing translations for escaped functions timestampadd
> and
> timestampdiff. There are two patchs : one for the code+test and one
> for
> the documentation.
>
> Xavier Poinsard.
> Index: jdbc2/AbstractJdbc2DatabaseMetaData.java
> ===================================================================
> RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/
> AbstractJdbc2DatabaseMetaData.java,v
> retrieving revision 1.29
> diff -u -r1.29 AbstractJdbc2DatabaseMetaData.java
> --- jdbc2/AbstractJdbc2DatabaseMetaData.java 3 Feb 2006 21:10:15
> -0000 1.29
> +++ jdbc2/AbstractJdbc2DatabaseMetaData.java 22 Mar 2006 08:32:51
> -0000
> @@ -477,7 +477,8 @@
> ','+EscapedFunctions.MONTH+
> ','+EscapedFunctions.MONTHNAME+','+EscapedFunctions.NOW+
> ','+EscapedFunctions.QUARTER+','+EscapedFunctions.SECOND+
> - ','+EscapedFunctions.WEEK+','+EscapedFunctions.YEAR;
> + ','+EscapedFunctions.WEEK+','+EscapedFunctions.YEAR+
> + ','+EscapedFunctions.TIMESTAMPADD
> +','+EscapedFunctions.TIMESTAMPDIFF;
> }
>
> /*
> Index: jdbc2/EscapedFunctions.java
> ===================================================================
> RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/
> EscapedFunctions.java,v
> retrieving revision 1.6
> diff -u -r1.6 EscapedFunctions.java
> --- jdbc2/EscapedFunctions.java 4 Jun 2005 18:24:08 -0000 1.6
> +++ jdbc2/EscapedFunctions.java 22 Mar 2006 08:32:51 -0000
> @@ -90,8 +90,23 @@
> public final static String SECOND="second";
> public final static String WEEK="week";
> public final static String YEAR="year";
> - // TODO : timestampadd and timestampdiff
> + // for timestampadd and timestampdiff the fractional part of
> second is not supported
> + // by the backend
> + public final static String TIMESTAMPADD="timestampadd";
> + public final static String TIMESTAMPDIFF="timestampdiff";
> +
> + // constants for timestampadd and timestampdiff
> + public final static String SQL_TSI_DAY="SQL_TSI_DAY";
> + public final static String
> SQL_TSI_FRAC_SECOND="SQL_TSI_FRAC_SECOND";
> + public final static String SQL_TSI_HOUR="SQL_TSI_HOUR";
> + public final static String SQL_TSI_MINUTE="SQL_TSI_MINUTE";
> + public final static String SQL_TSI_MONTH="SQL_TSI_MONTH";
> + public final static String SQL_TSI_QUARTER="SQL_TSI_QUARTER";
> + public final static String SQL_TSI_SECOND="SQL_TSI_SECOND";
> + public final static String SQL_TSI_WEEK="SQL_TSI_WEEK";
> + public final static String SQL_TSI_YEAR="SQL_TSI_YEAR";
>
> +
> // system functions
> public final static String DATABASE="database";
> public final static String IFNULL="ifnull";
> @@ -478,6 +493,79 @@
> return "extract(year from "+parsedArgs.get(0)+")";
> }
>
> + /** time stamp add */
> + public static String sqltimestampadd(List parsedArgs) throws
> SQLException{
> + if (parsedArgs.size()!=3){
> + throw new PSQLException(GT.tr("{0} function takes
> three and only three arguments.","timestampadd"),
> + PSQLState.SYNTAX_ERROR);
> + }
> + StringBuffer buf = new StringBuffer();
> + buf.append("(").append(EscapedFunctions.constantToInterval
> (parsedArgs.get(0).toString(),parsedArgs.get(1).toString()))
> + .append("+").append(parsedArgs.get(2)).append(")");
> + return buf.toString();
> + }
> +
> + private final static String constantToInterval(String
> type,String value)throws SQLException{
> + if (SQL_TSI_DAY.equalsIgnoreCase(type))
> + return "'"+value+" day'";
> + else if (SQL_TSI_SECOND.equalsIgnoreCase(type))
> + return "'"+value+" second'";
> + else if (SQL_TSI_HOUR.equalsIgnoreCase(type))
> + return "'"+value+" hour'";
> + else if (SQL_TSI_MINUTE.equalsIgnoreCase(type))
> + return "'"+value+" minute'";
> + else if (SQL_TSI_MONTH.equalsIgnoreCase(type))
> + return "'"+value+" month'";
> + else if (SQL_TSI_QUARTER.equalsIgnoreCase(type))
> + return "'3*"+value+" month'";
> + else if (SQL_TSI_WEEK.equalsIgnoreCase(type))
> + return "'"+value+" week'";
> + else if (SQL_TSI_YEAR.equalsIgnoreCase(type))
> + return "'"+value+" year'";
> + else if (SQL_TSI_FRAC_SECOND.equalsIgnoreCase(type))
> + throw new PSQLException(GT.tr("Interval {0} not yet
> implemented","SQL_TSI_FRAC_SECOND"),
> + PSQLState.SYNTAX_ERROR);
> + else throw new PSQLException(GT.tr("Interval {0} not yet
> implemented",type),
> + PSQLState.SYNTAX_ERROR);
> + }
> +
> +
> + /** time stamp diff */
> + public static String sqltimestampdiff(List parsedArgs) throws
> SQLException{
> + if (parsedArgs.size()!=3){
> + throw new PSQLException(GT.tr("{0} function takes
> three and only three arguments.","timestampdiff"),
> + PSQLState.SYNTAX_ERROR);
> + }
> + StringBuffer buf = new StringBuffer();
> + buf.append("extract( ").append
> (EscapedFunctions.constantToDatePart(parsedArgs.get(0).toString()))
> + .append(" from (").append(parsedArgs.get(2)).append
> ("-").append(parsedArgs.get(1)).append("))");
> + return buf.toString();
> + }
> +
> + private final static String constantToDatePart(String type)
> throws SQLException{
> + if (SQL_TSI_DAY.equalsIgnoreCase(type))
> + return "day";
> + else if (SQL_TSI_SECOND.equalsIgnoreCase(type))
> + return "second";
> + else if (SQL_TSI_HOUR.equalsIgnoreCase(type))
> + return "hour";
> + else if (SQL_TSI_MINUTE.equalsIgnoreCase(type))
> + return "minute";
> + else if (SQL_TSI_MONTH.equalsIgnoreCase(type))
> + return "month";
> + else if (SQL_TSI_QUARTER.equalsIgnoreCase(type))
> + return "quarter";
> + else if (SQL_TSI_WEEK.equalsIgnoreCase(type))
> + return "week";
> + else if (SQL_TSI_YEAR.equalsIgnoreCase(type))
> + return "year";
> + else if (SQL_TSI_FRAC_SECOND.equalsIgnoreCase(type))
> + throw new PSQLException(GT.tr("Interval {0} not yet
> implemented","SQL_TSI_FRAC_SECOND"),
> + PSQLState.SYNTAX_ERROR);
> + else throw new PSQLException(GT.tr("Interval {0} not yet
> implemented",type),
> + PSQLState.SYNTAX_ERROR);
> + }
> +
> /** database translation */
> public static String sqldatabase(List parsedArgs) throws
> SQLException{
> if (parsedArgs.size()!=0){
> Index: test/jdbc2/StatementTest.java
> ===================================================================
> RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/
> jdbc2/StatementTest.java,v
> retrieving revision 1.19
> diff -u -r1.19 StatementTest.java
> --- test/jdbc2/StatementTest.java 1 Feb 2006 18:52:13 -0000 1.19
> +++ test/jdbc2/StatementTest.java 22 Mar 2006 08:32:52 -0000
> @@ -297,6 +297,10 @@
> assertTrue(rs.next());
> // ensure sunday =>1 and monday =>2
> assertEquals(2,rs.getInt(5));
> +
> + rs = stmt.executeQuery("select {fn timestampdiff
> (SQL_TSI_DAY,{fn now()},{fn timestampadd(SQL_TSI_DAY,3,{fn now
> ()})})} ");
> + assertTrue(rs.next());
> + assertEquals(3,rs.getInt(1));
> }
>
> public void testSystemFunctions() throws SQLException
> Index: pgjdbc.xml
> ===================================================================
> RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/doc/pgjdbc.xml,v
> retrieving revision 1.27
> diff -u -r1.27 pgjdbc.xml
> --- pgjdbc.xml 24 Nov 2005 06:36:03 -0000 1.27
> +++ pgjdbc.xml 22 Mar 2006 08:33:21 -0000
> @@ -2040,6 +2040,18 @@
> <entry>extract(year from arg1)</entry>
> <entry></entry>
> </row>
> + <row>
> + <entry>timestampadd(argIntervalType,argCount,argTimeStamp)</
> entry>
> + <entry>yes</entry>
> + <entry>('(interval according to argIntervalType and
> argCount)'+argTimeStamp)</entry>
> + <entry>an argIntervalType value of
> <classname>SQL_TSI_FRAC_SECOND</classname> is not implemented since
> backend does not support it</entry>
> + </row>
> + <row>
> + <entry>timestampdiff
> (argIntervalType,argTimeStamp1,argTimeStamp2)</entry>
> + <entry>yes</entry>
> + <entry>extract((interval according to argIntervalType) from
> argTimeStamp2-argTimeStamp1 )</entry>
> + <entry>an argIntervalType value of
> <classname>SQL_TSI_FRAC_SECOND</classname> is not implemented since
> backend does not support it</entry>
> + </row>
> </tbody>
> </tgroup>
> </table>
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 6: explain analyze is your friend

Dave Cramer a écrit :
> Xavier,
>
> Thanks, any chance we could get a context diff instead of a plain diff ?

I should be better.

>
> Also I looked at it briefly, can you change the if (SQL_TSI_DAY. ....
> checks to check for "SQL_TSI" , before you even create the stringbuffer
> in sqltimestampdiff, and sqltimestampadd

done.

>
> Ideally it would be good to verify all of the functions in the test .

That's a good point since I found several unexpected problems with the
backend implementation of extract which makes severals SQL_TSI
unavailable for timestampdiff :

select extract(month from interval '92 days');
date_part
-----------
0
(1 row)

test=> select extract(year from interval '900 days');
date_part
-----------
0
(1 row)

test=> select extract(quarter from interval '900 days');
date_part
-----------
1
(1 row)

>
> Dave
>

Dave Cramer a écrit :
> Xavier,
>
> Sorry I wasn't clear what I was trying to avoid is this
>
> you have 10 (didn't count) or so compares for SQL_TSI_*
>
> so what I am suggesting is that you compare for SQL_TSI_ and then
> compare for DAY, HOUR, etc in the sqltimestampdiff/sqltimestampadd
>
> You can determine quickly before creating the StringBuffer if you
> should even check for DAY,HOUR, etc.
>
> Plus the overhead of redundant checking of SQL_TSI_ is avoided.
>
> I realize these are nitpicky kinds of things, but I've recently become
> aware of certain java performance issues
>

The optimized version ...

Xavier

The semantics of extract is that it will give the month/hour/minute
of the interval, not the number of months/hours/days of the interval

Dave
On 22-Mar-06, at 11:25 AM, Xavier Poinsard wrote:

> Dave Cramer a écrit :
>>> You are right, but I would not expect 0 with :
>>>
>>> select extract(month from
>>> (current_date+ interval '3 month'-current_date));
>>>
>>> date_part
>>> -----------
>>> 0
>>> (1 row)
>>
>>
>> This is still an interval basically you have
>>
>> date + interval - date which will be an interval. date - date is an
>> interval
>
> I think the interval implementation is bit deficient, since it only
> keeps one unit. Instead of keeping only the difference in days, it
> could
> have keep the information in month too.
>
> test=> select (current_date+interval '3 month'-current_date);
> ?column?
> ----------
> 92 days
> (1 row)
>
> test=> select extract(month from (interval '3 month'));
> date_part
> -----------
> 3
> (1 row)
>
> select extract(month from (interval '1 year'));
> date_part
> -----------
> 0
> (1 row)
>

Here are the updated patchs.

Dave Cramer a écrit :
> Xavier,
>
> So at this point do you want to remove some of the less correct escapes ?
>