Merge lp:~wiml-omni/libdrizzle/date-time into lp:libdrizzle

[Wim Lewis]

This fixes a handful of bugs related to dates and times in prepared statements, mostly found by the datetypes unit test.

This fixes a handful of bugs related to dates and times in prepared statements, mostly found by the datetypes unit test.

[Andrew Hutchings (linuxjedi)]

Many great fixes :)

review: Approve

[Andrew Hutchings (linuxjedi)]

So, there are two problems that flagged up when trying to merge this. I don't quite see how they are related to this branch yet, but:

Ubuntu 12.04:

../tests/unit/numbers.c:307: main Assertion 'FLOAT_EQ_EXACT(col_dblval, expect_dblval)' [ Floating-point values 291.000000 != 291.271111 (difference is -0.271111) ]
Row 1
  Column 2: 1 "1" 1.000000
  Column 3: 1 "1" 1.000000
  Column 4: 1 "1" 1.000000
  Column 5: 1 "1" 1.000000
  Column 6: 1 "1" 1.000000
  Column 7: 1 "1.000000" 1.000000
  Column 8: 1 "1.000000" 1.000000
Row 2
  Column 2: 127 "127" 127.000000
  Column 3: 32687 "32687" 32687.000000
  Column 4: 8388351 "8388351" 8388351.000000
  Column 5: 2147352575 "2147352575" 2147352575.000000
  Column 6: 9222246136947920895 "9222246136947920895" 9222246136947920896.000000
  Column 7: 443664 "443664.000000" 443664.000000
FAIL: tests/unit/numbers

Fedora 17:
/usr/bin/ld: tests/unit/numbers.o: undefined reference to symbol 'truncf@@GLIBC_2.2.5'
/usr/bin/ld: note: 'truncf@@GLIBC_2.2.5' is defined in DSO /lib64/libm.so.6 so try adding it to the linker command line
/lib64/libm.so.6: could not read symbols: Invalid operation
collect2: error: ld returned 1 exit status
make[1]: *** [tests/unit/numbers] Error 1

I'll look into these later this evening.

[Wim Lewis (wiml-omni)]

On 2 Apr 2013, at 11:36 AM, Andrew Hutchings wrote:
> So, there are two problems that flagged up when trying to merge this. I don't quite see how they are related to this branch yet, but:
>
> Ubuntu 12.04:
>
> ../tests/unit/numbers.c:307: main Assertion 'FLOAT_EQ_EXACT(col_dblval, expect_dblval)' [ Floating-point values 291.000000 != 291.271111 (difference is -0.271111) ]

Oho. I've fixed that, but haven't cleaned it up to commit yet, and had forgotten that numbers.c would hit it. (The problem, IIRC, is that drizzle_stmt_get_double() casts the value to uint64 or something before returning it as a double.)

> Fedora 17:
> /usr/bin/ld: tests/unit/numbers.o: undefined reference to symbol 'truncf@@GLIBC_2.2.5'
> /usr/bin/ld: note: 'truncf@@GLIBC_2.2.5' is defined in DSO /lib64/libm.so.6 so try adding it to the linker command line
> /lib64/libm.so.6: could not read symbols: Invalid operation
> collect2: error: ld returned 1 exit status
> make[1]: *** [tests/unit/numbers] Error 1

I guess the unit tests need to be linked against -lm if they're using trunc(). I missed that one since libc and libm are the same library on OSX.

[Wim Lewis (wiml-omni)]

I'm working on some cleaning up my fixes for the first of those problems and I'll commit them to this branch when I have a chance.

review: Needs Fixing

[Andrew Hutchings (linuxjedi)]

awesome, thanks.

When that is ready I'll fix the Fedora builder problem.

=== modified file 'libdrizzle-5.1/constants.h'
--- libdrizzle-5.1/constants.h	2013-03-05 03:27:07 +0000
+++ libdrizzle-5.1/constants.h	2013-04-01 17:45:26 +0000
@@ -621,6 +621,8 @@
   ((uint64_t)drizzle_get_byte4(((uint8_t *)__buffer)+4) << 32))
 
 /* Protocol packing macros. */
+#define drizzle_set_byte1(__buffer, __int) do { \
+    (__buffer)[0] = (uint8_t)(__int); } while (0)
 #define drizzle_set_byte2(__buffer, __int) do { \
   (__buffer)[0]= (uint8_t)((__int) & 0xFF); \
   (__buffer)[1]= (uint8_t)(((__int) >> 8) & 0xFF); } while (0)
 
=== modified file 'libdrizzle/pack.cc'
--- libdrizzle/pack.cc	2013-03-12 16:33:14 +0000
+++ libdrizzle/pack.cc	2013-04-01 17:45:26 +0000
@@ -193,71 +193,78 @@
 {
   uint8_t length= 0;
 
-  /* If nothing is set then we are sending a 0 length time */
-  if (time->day || time->hour || time->minute || time->second)
-  {
-    ptr[0]= (time->negative) ? 1 : 0;
-    drizzle_set_byte4(&ptr[1], time->day);
-    ptr[5]= (uint8_t) time->hour;
-    ptr[6]= time->minute;
-    ptr[7]= time->second;
-    length= 8;
-  }
   /* NOTE: MySQL has a bug here and doesn't follow this part of the protocol
    * when packing, we will for now, no idea if it works
    * */
   if (time->microsecond)
   {
-    drizzle_set_byte4(&ptr[8], time->microsecond);
+    drizzle_set_byte4(ptr+9, time->microsecond);
     length= 12;
   }
-  return ptr+length;
+
+  if (length || time->day || time->hour || time->minute || time->second)
+  {
+    ptr[1]= (time->negative) ? 1 : 0;
+    drizzle_set_byte4(ptr+2, time->day);
+    drizzle_set_byte1(ptr+6, time->hour);
+    drizzle_set_byte1(ptr+7, time->minute);
+    drizzle_set_byte1(ptr+8, time->second);
+    /* If no microseconds, then we are packing 8 bytes */
+    if (!length)
+      length= 8;
+  }
+
+  /* If nothing is set then we are sending a 0 length time */
+
+  drizzle_set_byte1(ptr, length);
+  return ptr + 1 + length;
 }
 
 unsigned char *drizzle_pack_datetime(drizzle_datetime_st *datetime, unsigned char *ptr)
 {
   uint8_t length= 0;
 
+  if (datetime->microsecond)
+  {
+    drizzle_set_byte4(ptr+8, datetime->microsecond);
+    length = 11;
+  }
+
+  if (length || datetime->hour || datetime->minute || datetime->second)
+  {
+    drizzle_set_byte1(ptr+5, datetime->hour);
+    drizzle_set_byte1(ptr+6, datetime->minute);
+    drizzle_set_byte1(ptr+7, datetime->second);
+    /* If only date+time is provided then we are packing 7 bytes */
+    if (!length)
+      length = 7;
+  }
+
+  if (length || datetime->year || datetime->month || datetime->day)
+  {
+    drizzle_set_byte2(ptr+1, datetime->year);
+    drizzle_set_byte1(ptr+3, datetime->month);
+    drizzle_set_byte1(ptr+4, datetime->day);
+    /* If only date is provided then we are packing 4 bytes */
+    if (!length)
+      length = 4;
+  }
+
   /* If nothing is set then we are sending a 0 length datetime */
 
-  /* If only date is provided then we are packing 4 bytes */
-  if (datetime->year || datetime->month || datetime->day)
-  {
-    drizzle_set_byte2(ptr, datetime->year);
-    ptr[2]= datetime->month;
-    ptr[3]= datetime->day;
-    length= 4;
-  }
-
-  if (datetime->hour || datetime->minute || datetime->second)
-  {
-    ptr[4]= (uint8_t) datetime->hour;
-    ptr[5]= datetime->minute;
-    ptr[6]= datetime->second;
-    length= 7;
-  }
-
-  if (datetime->microsecond)
-  {
-    drizzle_set_byte4(&ptr[7], datetime->microsecond);
-    length= 11;
-  }
-
-  return ptr + length;
+  drizzle_set_byte1(ptr, length);
+  return ptr + 1 + length;
 }
 
-void drizzle_unpack_time(drizzle_field_t field, size_t length, unsigned char *data)
+void drizzle_unpack_time(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime)
 {
-  drizzle_datetime_st *datetime= (drizzle_datetime_st*) data;
-  memset(datetime, 0, length);
+  memset(datetime, 0, sizeof(*datetime));
 
   if (length)
   {
     datetime->negative= field[0];
     datetime->day= drizzle_get_byte4(&field[1]);
     datetime->hour= field[5];
-    datetime->hour= datetime->day * 24;
-    datetime->day= 0;
     datetime->minute= field[6];
     datetime->second= field[7];
     if (length > 8)
@@ -267,10 +274,9 @@
   }
 }
 
-void drizzle_unpack_datetime(drizzle_field_t field, size_t length, unsigned char *data)
+void drizzle_unpack_datetime(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime)
 {
-  drizzle_datetime_st *datetime= (drizzle_datetime_st*) data;
-  memset(datetime, 0, length);
+  memset(datetime, 0, sizeof(*datetime));
 
   if (length)
   {
 
=== modified file 'libdrizzle/pack.h'
--- libdrizzle/pack.h	2013-03-06 18:05:42 +0000
+++ libdrizzle/pack.h	2013-04-01 17:45:26 +0000
@@ -79,9 +79,9 @@
 
 unsigned char *drizzle_pack_datetime(drizzle_datetime_st *datetime, unsigned char *ptr);
 
-void drizzle_unpack_time(drizzle_field_t field, size_t length, unsigned char *data);
+void drizzle_unpack_time(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime);
 
-void drizzle_unpack_datetime(drizzle_field_t field, size_t length, unsigned char *data);
+void drizzle_unpack_datetime(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime);
 
 /**
  * Unpack length-encoded string.
 
=== modified file 'libdrizzle/statement.cc'
--- libdrizzle/statement.cc	2013-03-21 10:44:02 +0000
+++ libdrizzle/statement.cc	2013-04-01 17:45:26 +0000
@@ -471,13 +471,13 @@
           break;
         case DRIZZLE_COLUMN_TYPE_TIME:
           param->data= param->data_buffer;
-          drizzle_unpack_time(row[column_counter], param->length, (unsigned char*)param->data);
+          drizzle_unpack_time(row[column_counter], param->length, (drizzle_datetime_st *)param->data);
           break;
         case DRIZZLE_COLUMN_TYPE_DATE:
         case DRIZZLE_COLUMN_TYPE_DATETIME:
         case DRIZZLE_COLUMN_TYPE_TIMESTAMP:
           param->data= param->data_buffer;
-          drizzle_unpack_datetime(row[column_counter], param->length, (unsigned char*)param->data);
+          drizzle_unpack_datetime(row[column_counter], param->length, (drizzle_datetime_st *)param->data);
           break;
         case DRIZZLE_COLUMN_TYPE_TINY_BLOB:
         case DRIZZLE_COLUMN_TYPE_MEDIUM_BLOB:
 
=== modified file 'libdrizzle/statement_param.cc'
--- libdrizzle/statement_param.cc	2013-03-11 17:33:00 +0000
+++ libdrizzle/statement_param.cc	2013-04-01 17:45:26 +0000
@@ -129,6 +129,8 @@
   drizzle_datetime_st *time;
   time= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
 
+  bzero(time, sizeof(*time));
+
   time->negative= is_negative;
   time->day= days;
   time->hour= hours;
@@ -145,6 +147,8 @@
   drizzle_datetime_st *timestamp;
   timestamp= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
 
+  bzero(timestamp, sizeof(*timestamp));
+
   timestamp->negative= false;
   timestamp->year= year;
   timestamp->day= day;
@@ -156,7 +160,7 @@
   timestamp->microsecond= microseconds;
 
   /* Length not important because we will figure that out when packing */ 
-  return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_TIME, timestamp, 0, false);
+  return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_TIMESTAMP, timestamp, 0, false);
 }
 
 bool drizzle_stmt_get_is_null_from_name(drizzle_stmt_st *stmt, const char *column_name, drizzle_return_t *ret_ptr)
@@ -619,29 +623,46 @@
 {
   /* Max time is -HHH:MM:SS.ssssss + NUL = 17 */
   char* buffer= param->data_buffer + 50;
-  if (time->microsecond == 0)
-  {
-    snprintf(buffer, 17, "%s%"PRIu16":%"PRIu8":%"PRIu8, (time->negative) ? "-" : "", time->hour, time->minute, time->second);
-  }
-  else
-  {
-    snprintf(buffer, 17, "%s%"PRIu16":%"PRIu8":%"PRIu8".%"PRIu32, (time->negative) ? "-" : "", time->hour, time->minute, time->second, time->microsecond);
-  }
+  int buffersize = 17;
+  int used = 0;
+    
+  /* Values are transferred with days separated from hours, but presented with days folded into hours. */
+  used = snprintf(buffer, buffersize-used, "%s%02u:%02"PRIu8":%02"PRIu8, (time->negative) ? "-" : "", time->hour + 24 * time->day, time->minute, time->second);
+
+  /* TODO: the existence (and length) of the decimals should be decided based on the number of fields sent by the server or possibly the column's "decimals" value, not by whether the microseconds are 0 */
+  if (time->microsecond)
+    used += snprintf(buffer+used, buffersize-used, ".%06" PRIu32, time->microsecond);
+  
+  assert(used < buffersize);
+    
   return buffer;
 }
 
 char *timestamp_to_string(drizzle_bind_st *param, drizzle_datetime_st *timestamp)
 {
-  /* Max timestamp is YYYY-MM-DD HH:MM:SS.ssssss + NUL = 26 */
+  /* Max timestamp is YYYY-MM-DD HH:MM:SS.ssssss + NUL = 27 */
   char* buffer= param->data_buffer + 50;
-  if (timestamp->microsecond == 0)
-  {
-    snprintf(buffer, 26, "%"PRIu16"-%"PRIu8"-%"PRIu32" %"PRIu16":%"PRIu8":%"PRIu8, timestamp->year, timestamp->month, timestamp->day, timestamp->hour, timestamp->minute, timestamp->second);
-  }
-  else
-  {
-    snprintf(buffer, 26, "%"PRIu16"-%"PRIu8"-%"PRIu32" %"PRIu16":%"PRIu8":%"PRIu8".%"PRIu32, timestamp->year, timestamp->month, timestamp->day, timestamp->hour, timestamp->minute, timestamp->second, timestamp->microsecond);
-  }
+  int buffersize = 27;
+  int used = 0;
+  
+  used += snprintf(buffer, buffersize-used, "%"PRIu16"-%02"PRIu8"-%02"PRIu32,
+     timestamp->year, timestamp->month, timestamp->day);
+  assert(used < buffersize);
+  
+  if (param->type == DRIZZLE_COLUMN_TYPE_DATE)
+    return buffer;
+  
+  used += snprintf(buffer+used, buffersize-used, " %02"PRIu16":%02"PRIu8":%02"PRIu8,
+    timestamp->hour, timestamp->minute, timestamp->second);
+
+  /* TODO: the existence (and length) of the decimals should be decided based on the number of fields sent by the server or possibly the column's "decimals" value, not by whether the microseconds are 0 */
+  if (timestamp->microsecond)
+  {
+    used += snprintf(buffer+used, buffersize-used, ".%06"PRIu32, timestamp->microsecond);
+  }
+  
+  assert(used < buffersize);
+  
   return buffer;
 }