Proposed patch: make SQL interval-literal syntax work per spec - Mailing list pgsql-hackers

Over in that TPC-H thread, I was bemoaning once again the never-finished
support for SQL-spec interval literals.  I decided to go look at exactly
how unfinished it was, and it turns out that it's actually pretty close.
Hence the attached proposed patch ;-)

The main gating factor is that coerce_type doesn't want to pass typmod
through to the datatype input function when converting a literal
constant.  This is necessary for certain types like char and varchar
but loses badly for interval.  I have a feeling that we changed that
behavior after Tom Lockhart left the project, which may mean that
interval wasn't quite as broken when he left it as it is today.
Anyway, the attached patch simply hardwires a special case for INTERVAL.
Given that this is reflective of a special case in the standard, and
that there's no very good reason for anyone else to design a datatype
that acts this way, I don't feel too bad about such a hack; but has
anyone got a better idea?

After that it's just a matter of getting DecodeInterval to do the
right things; and it turns out that about half the logic for SQL-spec
input syntax was there already.  Almost the only thing I had to change
was the code to decide what a plain integer field at the right end of
the input means.

The patch includes regression test changes that illustrate what it does.
I am not sure about some of the corner cases --- anyone want to see if
their understanding of the spec for <interval string> is different?

There is still some unfinished business if anyone wants to make it
really exactly 100% spec compliant.  In particular the spec seems to
allow a minus sign *outside* the string literal, and if I'm reading it
right, a precision spec in combination with field restrictions ought to
look like INTERVAL '...' DAY TO SECOND(3) not INTERVAL(3) '...' DAY TO
SECOND.  However, for these you'll get a syntax error instead of
silently wrong answers if you try to use the other syntax, so it's not
quite as pernicious as the matters addressed here.

            regards, tom lane

Index: src/backend/parser/parse_coerce.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_coerce.c,v
retrieving revision 2.166
diff -c -r2.166 parse_coerce.c
*** src/backend/parser/parse_coerce.c    1 Sep 2008 20:42:44 -0000    2.166
--- src/backend/parser/parse_coerce.c    9 Sep 2008 23:47:59 -0000
***************
*** 179,184 ****
--- 179,185 ----
          Const       *newcon = makeNode(Const);
          Oid            baseTypeId;
          int32        baseTypeMod;
+         int32        inputTypeMod;
          Type        targetType;
          ParseCallbackState pcbstate;

***************
*** 190,202 ****
           * what we want here.  The needed check will be applied properly
           * inside coerce_to_domain().
           */
!         baseTypeMod = -1;
          baseTypeId = getBaseTypeAndTypmod(targetTypeId, &baseTypeMod);

          targetType = typeidType(baseTypeId);

          newcon->consttype = baseTypeId;
!         newcon->consttypmod = -1;
          newcon->constlen = typeLen(targetType);
          newcon->constbyval = typeByVal(targetType);
          newcon->constisnull = con->constisnull;
--- 191,217 ----
           * what we want here.  The needed check will be applied properly
           * inside coerce_to_domain().
           */
!         baseTypeMod = targetTypeMod;
          baseTypeId = getBaseTypeAndTypmod(targetTypeId, &baseTypeMod);

+         /*
+          * For most types we pass typmod -1 to the input routine, because
+          * existing input routines follow implicit-coercion semantics for
+          * length checks, which is not always what we want here.  Any length
+          * constraint will be applied later by our caller.  An exception
+          * however is the INTERVAL type, for which we *must* pass the typmod
+          * or it won't be able to obey the bizarre SQL-spec input rules.
+          * (Ugly as sin, but so is this part of the spec...)
+          */
+         if (baseTypeId == INTERVALOID)
+             inputTypeMod = baseTypeMod;
+         else
+             inputTypeMod = -1;
+
          targetType = typeidType(baseTypeId);

          newcon->consttype = baseTypeId;
!         newcon->consttypmod = inputTypeMod;
          newcon->constlen = typeLen(targetType);
          newcon->constbyval = typeByVal(targetType);
          newcon->constisnull = con->constisnull;
***************
*** 215,234 ****
          setup_parser_errposition_callback(&pcbstate, pstate, con->location);

          /*
-          * We pass typmod -1 to the input routine, primarily because existing
-          * input routines follow implicit-coercion semantics for length
-          * checks, which is not always what we want here. Any length
-          * constraint will be applied later by our caller.
-          *
           * We assume here that UNKNOWN's internal representation is the same
           * as CSTRING.
           */
          if (!con->constisnull)
              newcon->constvalue = stringTypeDatum(targetType,
                                              DatumGetCString(con->constvalue),
!                                                  -1);
          else
!             newcon->constvalue = stringTypeDatum(targetType, NULL, -1);

          cancel_parser_errposition_callback(&pcbstate);

--- 230,246 ----
          setup_parser_errposition_callback(&pcbstate, pstate, con->location);

          /*
           * We assume here that UNKNOWN's internal representation is the same
           * as CSTRING.
           */
          if (!con->constisnull)
              newcon->constvalue = stringTypeDatum(targetType,
                                              DatumGetCString(con->constvalue),
!                                                  inputTypeMod);
          else
!             newcon->constvalue = stringTypeDatum(targetType,
!                                                  NULL,
!                                                  inputTypeMod);

          cancel_parser_errposition_callback(&pcbstate);

Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.190
diff -c -r1.190 datetime.c
*** src/backend/utils/adt/datetime.c    9 Jun 2008 19:34:02 -0000    1.190
--- src/backend/utils/adt/datetime.c    9 Sep 2008 23:48:00 -0000
***************
*** 35,42 ****
  static int DecodeNumberField(int len, char *str,
                    int fmask, int *tmask,
                    struct pg_tm * tm, fsec_t *fsec, bool *is2digits);
! static int DecodeTime(char *str, int fmask, int *tmask,
!            struct pg_tm * tm, fsec_t *fsec);
  static int    DecodeTimezone(char *str, int *tzp);
  static const datetkn *datebsearch(const char *key, const datetkn *base, int nel);
  static int    DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
--- 35,42 ----
  static int DecodeNumberField(int len, char *str,
                    int fmask, int *tmask,
                    struct pg_tm * tm, fsec_t *fsec, bool *is2digits);
! static int DecodeTime(char *str, int fmask, int range,
!            int *tmask, struct pg_tm * tm, fsec_t *fsec);
  static int    DecodeTimezone(char *str, int *tzp);
  static const datetkn *datebsearch(const char *key, const datetkn *base, int nel);
  static int    DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
***************
*** 832,838 ****
                  break;

              case DTK_TIME:
!                 dterr = DecodeTime(field[i], fmask, &tmask, tm, fsec);
                  if (dterr)
                      return dterr;

--- 832,839 ----
                  break;

              case DTK_TIME:
!                 dterr = DecodeTime(field[i], fmask, INTERVAL_FULL_RANGE,
!                                    &tmask, tm, fsec);
                  if (dterr)
                      return dterr;

***************
*** 1563,1568 ****
--- 1564,1570 ----

              case DTK_TIME:
                  dterr = DecodeTime(field[i], (fmask | DTK_DATE_M),
+                                    INTERVAL_FULL_RANGE,
                                     &tmask, tm, fsec);
                  if (dterr)
                      return dterr;
***************
*** 2224,2230 ****
   * used to represent time spans.
   */
  static int
! DecodeTime(char *str, int fmask, int *tmask, struct pg_tm * tm, fsec_t *fsec)
  {
      char       *cp;

--- 2226,2233 ----
   * used to represent time spans.
   */
  static int
! DecodeTime(char *str, int fmask, int range,
!            int *tmask, struct pg_tm * tm, fsec_t *fsec)
  {
      char       *cp;

***************
*** 2245,2250 ****
--- 2248,2260 ----
      {
          tm->tm_sec = 0;
          *fsec = 0;
+         /* If it's a MINUTE TO SECOND interval, take 2 fields as being mm:ss */
+         if (range == (INTERVAL_MASK(MINUTE) | INTERVAL_MASK(SECOND)))
+         {
+             tm->tm_sec = tm->tm_min;
+             tm->tm_min = tm->tm_hour;
+             tm->tm_hour = 0;
+         }
      }
      else if (*cp != ':')
          return DTERR_BAD_FORMAT;
***************
*** 2705,2711 ****
   *    preceding an hh:mm:ss field. - thomas 1998-04-30
   */
  int
! DecodeInterval(char **field, int *ftype, int nf, int *dtype, struct pg_tm * tm, fsec_t *fsec)
  {
      bool        is_before = FALSE;
      char       *cp;
--- 2715,2722 ----
   *    preceding an hh:mm:ss field. - thomas 1998-04-30
   */
  int
! DecodeInterval(char **field, int *ftype, int nf, int range,
!                int *dtype, struct pg_tm * tm, fsec_t *fsec)
  {
      bool        is_before = FALSE;
      char       *cp;
***************
*** 2734,2740 ****
          switch (ftype[i])
          {
              case DTK_TIME:
!                 dterr = DecodeTime(field[i], fmask, &tmask, tm, fsec);
                  if (dterr)
                      return dterr;
                  type = DTK_DAY;
--- 2745,2752 ----
          switch (ftype[i])
          {
              case DTK_TIME:
!                 dterr = DecodeTime(field[i], fmask, range,
!                                    &tmask, tm, fsec);
                  if (dterr)
                      return dterr;
                  type = DTK_DAY;
***************
*** 2757,2763 ****
                  while (*cp != '\0' && *cp != ':' && *cp != '.')
                      cp++;
                  if (*cp == ':' &&
!                     DecodeTime(field[i] + 1, fmask, &tmask, tm, fsec) == 0)
                  {
                      if (*field[i] == '-')
                      {
--- 2769,2776 ----
                  while (*cp != '\0' && *cp != ':' && *cp != '.')
                      cp++;
                  if (*cp == ':' &&
!                     DecodeTime(field[i] + 1, fmask, INTERVAL_FULL_RANGE,
!                                &tmask, tm, fsec) == 0)
                  {
                      if (*field[i] == '-')
                      {
***************
*** 2796,2814 ****
                          type = DTK_HOUR;
                      }
                  }
!                 /* DROP THROUGH */

              case DTK_DATE:
              case DTK_NUMBER:
                  errno = 0;
                  val = strtoi(field[i], &cp, 10);
                  if (errno == ERANGE)
                      return DTERR_FIELD_OVERFLOW;

!                 if (type == IGNORE_DTF)
!                     type = DTK_SECOND;

!                 if (*cp == '.')
                  {
                      fval = strtod(cp, &cp);
                      if (*cp != '\0')
--- 2809,2874 ----
                          type = DTK_HOUR;
                      }
                  }
!                 /* FALL THROUGH */

              case DTK_DATE:
              case DTK_NUMBER:
+                 if (type == IGNORE_DTF)
+                 {
+                     /* use typmod to decide what rightmost integer field is */
+                     switch (range)
+                     {
+                         case INTERVAL_MASK(YEAR):
+                             type = DTK_YEAR;
+                             break;
+                         case INTERVAL_MASK(MONTH):
+                         case INTERVAL_MASK(YEAR) | INTERVAL_MASK(MONTH):
+                             type = DTK_MONTH;
+                             break;
+                         case INTERVAL_MASK(DAY):
+                             type = DTK_DAY;
+                             break;
+                         case INTERVAL_MASK(HOUR):
+                         case INTERVAL_MASK(DAY) | INTERVAL_MASK(HOUR):
+                         case INTERVAL_MASK(DAY) | INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE):
+                         case INTERVAL_MASK(DAY) | INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE) |
INTERVAL_MASK(SECOND):
+                             type = DTK_HOUR;
+                             break;
+                         case INTERVAL_MASK(MINUTE):
+                         case INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE):
+                             type = DTK_MINUTE;
+                             break;
+                         case INTERVAL_MASK(SECOND):
+                         case INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE) | INTERVAL_MASK(SECOND):
+                         case INTERVAL_MASK(MINUTE) | INTERVAL_MASK(SECOND):
+                             type = DTK_SECOND;
+                             break;
+                         default:
+                             type = DTK_SECOND;
+                             break;
+                     }
+                 }
+
                  errno = 0;
                  val = strtoi(field[i], &cp, 10);
                  if (errno == ERANGE)
                      return DTERR_FIELD_OVERFLOW;

!                 if (*cp == '-')
!                 {
!                     /* SQL "years-months" syntax */
!                     int        val2;

!                     val2 = strtoi(cp + 1, &cp, 10);
!                     if (errno == ERANGE || val2 < 0 || val2 >= MONTHS_PER_YEAR)
!                         return DTERR_FIELD_OVERFLOW;
!                     if (*cp != '\0')
!                         return DTERR_BAD_FORMAT;
!                     type = DTK_MONTH;
!                     val = val * MONTHS_PER_YEAR + val2;
!                     fval = 0;
!                 }
!                 else if (*cp == '.')
                  {
                      fval = strtod(cp, &cp);
                      if (*cp != '\0')
***************
*** 2896,2901 ****
--- 2956,2962 ----
  #endif
                          }
                          tmask = DTK_M(HOUR);
+                         type = DTK_DAY;
                          break;

                      case DTK_DAY:
Index: src/backend/utils/adt/nabstime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/nabstime.c,v
retrieving revision 1.155
diff -c -r1.155 nabstime.c
*** src/backend/utils/adt/nabstime.c    25 Mar 2008 22:42:44 -0000    1.155
--- src/backend/utils/adt/nabstime.c    9 Sep 2008 23:48:00 -0000
***************
*** 632,638 ****
      dterr = ParseDateTime(str, workbuf, sizeof(workbuf),
                            field, ftype, MAXDATEFIELDS, &nf);
      if (dterr == 0)
!         dterr = DecodeInterval(field, ftype, nf, &dtype, tm, &fsec);
      if (dterr != 0)
      {
          if (dterr == DTERR_FIELD_OVERFLOW)
--- 632,639 ----
      dterr = ParseDateTime(str, workbuf, sizeof(workbuf),
                            field, ftype, MAXDATEFIELDS, &nf);
      if (dterr == 0)
!         dterr = DecodeInterval(field, ftype, nf, INTERVAL_FULL_RANGE,
!                                &dtype, tm, &fsec);
      if (dterr != 0)
      {
          if (dterr == DTERR_FIELD_OVERFLOW)
Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.190
diff -c -r1.190 timestamp.c
*** src/backend/utils/adt/timestamp.c    7 Jul 2008 18:09:46 -0000    1.190
--- src/backend/utils/adt/timestamp.c    9 Sep 2008 23:48:00 -0000
***************
*** 604,609 ****
--- 604,610 ----
                 *tm = &tt;
      int            dtype;
      int            nf;
+     int            range;
      int            dterr;
      char       *field[MAXDATEFIELDS];
      int            ftype[MAXDATEFIELDS];
***************
*** 617,626 ****
      tm->tm_sec = 0;
      fsec = 0;

      dterr = ParseDateTime(str, workbuf, sizeof(workbuf), field,
                            ftype, MAXDATEFIELDS, &nf);
      if (dterr == 0)
!         dterr = DecodeInterval(field, ftype, nf, &dtype, tm, &fsec);
      if (dterr != 0)
      {
          if (dterr == DTERR_FIELD_OVERFLOW)
--- 618,632 ----
      tm->tm_sec = 0;
      fsec = 0;

+     if (typmod >= 0)
+         range = INTERVAL_RANGE(typmod);
+     else
+         range = INTERVAL_FULL_RANGE;
+
      dterr = ParseDateTime(str, workbuf, sizeof(workbuf), field,
                            ftype, MAXDATEFIELDS, &nf);
      if (dterr == 0)
!         dterr = DecodeInterval(field, ftype, nf, range, &dtype, tm, &fsec);
      if (dterr != 0)
      {
          if (dterr == DTERR_FIELD_OVERFLOW)
***************
*** 945,951 ****
       * Unspecified range and precision? Then not necessary to adjust. Setting
       * typmod to -1 is the convention for all types.
       */
!     if (typmod != -1)
      {
          int            range = INTERVAL_RANGE(typmod);
          int            precision = INTERVAL_PRECISION(typmod);
--- 951,957 ----
       * Unspecified range and precision? Then not necessary to adjust. Setting
       * typmod to -1 is the convention for all types.
       */
!     if (typmod >= 0)
      {
          int            range = INTERVAL_RANGE(typmod);
          int            precision = INTERVAL_PRECISION(typmod);
Index: src/include/utils/datetime.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/datetime.h,v
retrieving revision 1.69
diff -c -r1.69 datetime.h
*** src/include/utils/datetime.h    1 Jan 2008 19:45:59 -0000    1.69
--- src/include/utils/datetime.h    9 Sep 2008 23:48:00 -0000
***************
*** 290,296 ****
                 int nf, int *dtype,
                 struct pg_tm * tm, fsec_t *fsec, int *tzp);
  extern int DecodeInterval(char **field, int *ftype,
!                int nf, int *dtype,
                 struct pg_tm * tm, fsec_t *fsec);
  extern void DateTimeParseError(int dterr, const char *str,
                     const char *datatype);
--- 290,296 ----
                 int nf, int *dtype,
                 struct pg_tm * tm, fsec_t *fsec, int *tzp);
  extern int DecodeInterval(char **field, int *ftype,
!                int nf, int range, int *dtype,
                 struct pg_tm * tm, fsec_t *fsec);
  extern void DateTimeParseError(int dterr, const char *str,
                     const char *datatype);
Index: src/test/regress/expected/interval.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/interval.out,v
retrieving revision 1.20
diff -c -r1.20 interval.out
*** src/test/regress/expected/interval.out    1 Sep 2008 20:42:46 -0000    1.20
--- src/test/regress/expected/interval.out    9 Sep 2008 23:48:00 -0000
***************
*** 361,363 ****
--- 361,520 ----
  ERROR:  invalid input syntax for type interval: "1:20:05 5 microseconds"
  LINE 1: SELECT '1:20:05 5 microseconds'::interval;
                 ^
+ SELECT interval '1-2';  -- SQL year-month literal
+    interval
+ ---------------
+  1 year 2 mons
+ (1 row)
+
+ -- test SQL-spec syntaxes for restricted field sets
+ SELECT interval '1' year;
+  interval
+ ----------
+  1 year
+ (1 row)
+
+ SELECT interval '2' month;
+  interval
+ ----------
+  2 mons
+ (1 row)
+
+ SELECT interval '3' day;
+  interval
+ ----------
+  3 days
+ (1 row)
+
+ SELECT interval '4' hour;
+  interval
+ ----------
+  04:00:00
+ (1 row)
+
+ SELECT interval '5' minute;
+  interval
+ ----------
+  00:05:00
+ (1 row)
+
+ SELECT interval '6' second;
+  interval
+ ----------
+  00:00:06
+ (1 row)
+
+ SELECT interval '1' year to month;
+  interval
+ ----------
+  1 mon
+ (1 row)
+
+ SELECT interval '1-2' year to month;
+    interval
+ ---------------
+  1 year 2 mons
+ (1 row)
+
+ SELECT interval '1 2' day to hour;
+     interval
+ ----------------
+  1 day 02:00:00
+ (1 row)
+
+ SELECT interval '1 2:03' day to hour;
+     interval
+ ----------------
+  1 day 02:00:00
+ (1 row)
+
+ SELECT interval '1 2:03:04' day to hour;
+     interval
+ ----------------
+  1 day 02:00:00
+ (1 row)
+
+ SELECT interval '1 2' day to minute;
+     interval
+ ----------------
+  1 day 02:00:00
+ (1 row)
+
+ SELECT interval '1 2:03' day to minute;
+     interval
+ ----------------
+  1 day 02:03:00
+ (1 row)
+
+ SELECT interval '1 2:03:04' day to minute;
+     interval
+ ----------------
+  1 day 02:03:00
+ (1 row)
+
+ SELECT interval '1 2' day to second;
+     interval
+ ----------------
+  1 day 02:00:00
+ (1 row)
+
+ SELECT interval '1 2:03' day to second;
+     interval
+ ----------------
+  1 day 02:03:00
+ (1 row)
+
+ SELECT interval '1 2:03:04' day to second;
+     interval
+ ----------------
+  1 day 02:03:04
+ (1 row)
+
+ SELECT interval '1 2' hour to minute;
+ ERROR:  invalid input syntax for type interval: "1 2"
+ LINE 1: SELECT interval '1 2' hour to minute;
+                         ^
+ SELECT interval '1 2:03' hour to minute;
+  interval
+ ----------
+  02:03:00
+ (1 row)
+
+ SELECT interval '1 2:03:04' hour to minute;
+  interval
+ ----------
+  02:03:00
+ (1 row)
+
+ SELECT interval '1 2' hour to second;
+ ERROR:  invalid input syntax for type interval: "1 2"
+ LINE 1: SELECT interval '1 2' hour to second;
+                         ^
+ SELECT interval '1 2:03' hour to second;
+  interval
+ ----------
+  02:03:00
+ (1 row)
+
+ SELECT interval '1 2:03:04' hour to second;
+  interval
+ ----------
+  02:03:04
+ (1 row)
+
+ SELECT interval '1 2' minute to second;
+ ERROR:  invalid input syntax for type interval: "1 2"
+ LINE 1: SELECT interval '1 2' minute to second;
+                         ^
+ SELECT interval '1 2:03' minute to second;
+  interval
+ ----------
+  00:02:03
+ (1 row)
+
+ SELECT interval '1 2:03:04' minute to second;
+  interval
+ ----------
+  00:03:04
+ (1 row)
+
Index: src/test/regress/sql/interval.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/interval.sql,v
retrieving revision 1.12
diff -c -r1.12 interval.sql
*** src/test/regress/sql/interval.sql    29 May 2007 04:58:43 -0000    1.12
--- src/test/regress/sql/interval.sql    9 Sep 2008 23:48:00 -0000
***************
*** 126,129 ****
  SELECT '1 second 2 seconds'::interval;              -- error
  SELECT '10 milliseconds 20 milliseconds'::interval; -- error
  SELECT '5.5 seconds 3 milliseconds'::interval;      -- error
! SELECT '1:20:05 5 microseconds'::interval;          -- error
\ No newline at end of file
--- 126,158 ----
  SELECT '1 second 2 seconds'::interval;              -- error
  SELECT '10 milliseconds 20 milliseconds'::interval; -- error
  SELECT '5.5 seconds 3 milliseconds'::interval;      -- error
! SELECT '1:20:05 5 microseconds'::interval;          -- error
! SELECT interval '1-2';  -- SQL year-month literal
!
! -- test SQL-spec syntaxes for restricted field sets
! SELECT interval '1' year;
! SELECT interval '2' month;
! SELECT interval '3' day;
! SELECT interval '4' hour;
! SELECT interval '5' minute;
! SELECT interval '6' second;
! SELECT interval '1' year to month;
! SELECT interval '1-2' year to month;
! SELECT interval '1 2' day to hour;
! SELECT interval '1 2:03' day to hour;
! SELECT interval '1 2:03:04' day to hour;
! SELECT interval '1 2' day to minute;
! SELECT interval '1 2:03' day to minute;
! SELECT interval '1 2:03:04' day to minute;
! SELECT interval '1 2' day to second;
! SELECT interval '1 2:03' day to second;
! SELECT interval '1 2:03:04' day to second;
! SELECT interval '1 2' hour to minute;
! SELECT interval '1 2:03' hour to minute;
! SELECT interval '1 2:03:04' hour to minute;
! SELECT interval '1 2' hour to second;
! SELECT interval '1 2:03' hour to second;
! SELECT interval '1 2:03:04' hour to second;
! SELECT interval '1 2' minute to second;
! SELECT interval '1 2:03' minute to second;
! SELECT interval '1 2:03:04' minute to second;