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

Proposed by Wim Lewis on 2013-04-01
Status: Merged
Approved by: Andrew Hutchings on 2013-04-02
Approved revision: 115
Merged at revision: 111
Proposed branch: lp:~wiml-omni/libdrizzle/date-time
Merge into: lp:libdrizzle
Diff against target: 279 lines (+95/-66)
5 files modified
libdrizzle-5.1/constants.h (+2/-0)
libdrizzle/pack.cc (+50/-44)
libdrizzle/pack.h (+2/-2)
libdrizzle/statement.cc (+2/-2)
libdrizzle/statement_param.cc (+39/-18)
To merge this branch: bzr merge lp:~wiml-omni/libdrizzle/date-time
Reviewer Review Type Date Requested Status
Wim Lewis (community) Needs Fixing on 2013-04-10
Andrew Hutchings 2013-04-01 Approve on 2013-04-02
Review via email: mp+156381@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Andrew Hutchings (linuxjedi) wrote :

Many great fixes :)

review: Approve
Andrew Hutchings (linuxjedi) 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) ]
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) wrote :

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) wrote :

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) wrote :

awesome, thanks.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libdrizzle-5.1/constants.h'
2--- libdrizzle-5.1/constants.h 2013-03-05 03:27:07 +0000
3+++ libdrizzle-5.1/constants.h 2013-04-01 17:45:26 +0000
4@@ -621,6 +621,8 @@
5 ((uint64_t)drizzle_get_byte4(((uint8_t *)__buffer)+4) << 32))
6
7 /* Protocol packing macros. */
8+#define drizzle_set_byte1(__buffer, __int) do { \
9+ (__buffer)[0] = (uint8_t)(__int); } while (0)
10 #define drizzle_set_byte2(__buffer, __int) do { \
11 (__buffer)[0]= (uint8_t)((__int) & 0xFF); \
12 (__buffer)[1]= (uint8_t)(((__int) >> 8) & 0xFF); } while (0)
13
14=== modified file 'libdrizzle/pack.cc'
15--- libdrizzle/pack.cc 2013-03-12 16:33:14 +0000
16+++ libdrizzle/pack.cc 2013-04-01 17:45:26 +0000
17@@ -193,71 +193,78 @@
18 {
19 uint8_t length= 0;
20
21- /* If nothing is set then we are sending a 0 length time */
22- if (time->day || time->hour || time->minute || time->second)
23- {
24- ptr[0]= (time->negative) ? 1 : 0;
25- drizzle_set_byte4(&ptr[1], time->day);
26- ptr[5]= (uint8_t) time->hour;
27- ptr[6]= time->minute;
28- ptr[7]= time->second;
29- length= 8;
30- }
31 /* NOTE: MySQL has a bug here and doesn't follow this part of the protocol
32 * when packing, we will for now, no idea if it works
33 * */
34 if (time->microsecond)
35 {
36- drizzle_set_byte4(&ptr[8], time->microsecond);
37+ drizzle_set_byte4(ptr+9, time->microsecond);
38 length= 12;
39 }
40- return ptr+length;
41+
42+ if (length || time->day || time->hour || time->minute || time->second)
43+ {
44+ ptr[1]= (time->negative) ? 1 : 0;
45+ drizzle_set_byte4(ptr+2, time->day);
46+ drizzle_set_byte1(ptr+6, time->hour);
47+ drizzle_set_byte1(ptr+7, time->minute);
48+ drizzle_set_byte1(ptr+8, time->second);
49+ /* If no microseconds, then we are packing 8 bytes */
50+ if (!length)
51+ length= 8;
52+ }
53+
54+ /* If nothing is set then we are sending a 0 length time */
55+
56+ drizzle_set_byte1(ptr, length);
57+ return ptr + 1 + length;
58 }
59
60 unsigned char *drizzle_pack_datetime(drizzle_datetime_st *datetime, unsigned char *ptr)
61 {
62 uint8_t length= 0;
63
64+ if (datetime->microsecond)
65+ {
66+ drizzle_set_byte4(ptr+8, datetime->microsecond);
67+ length = 11;
68+ }
69+
70+ if (length || datetime->hour || datetime->minute || datetime->second)
71+ {
72+ drizzle_set_byte1(ptr+5, datetime->hour);
73+ drizzle_set_byte1(ptr+6, datetime->minute);
74+ drizzle_set_byte1(ptr+7, datetime->second);
75+ /* If only date+time is provided then we are packing 7 bytes */
76+ if (!length)
77+ length = 7;
78+ }
79+
80+ if (length || datetime->year || datetime->month || datetime->day)
81+ {
82+ drizzle_set_byte2(ptr+1, datetime->year);
83+ drizzle_set_byte1(ptr+3, datetime->month);
84+ drizzle_set_byte1(ptr+4, datetime->day);
85+ /* If only date is provided then we are packing 4 bytes */
86+ if (!length)
87+ length = 4;
88+ }
89+
90 /* If nothing is set then we are sending a 0 length datetime */
91
92- /* If only date is provided then we are packing 4 bytes */
93- if (datetime->year || datetime->month || datetime->day)
94- {
95- drizzle_set_byte2(ptr, datetime->year);
96- ptr[2]= datetime->month;
97- ptr[3]= datetime->day;
98- length= 4;
99- }
100-
101- if (datetime->hour || datetime->minute || datetime->second)
102- {
103- ptr[4]= (uint8_t) datetime->hour;
104- ptr[5]= datetime->minute;
105- ptr[6]= datetime->second;
106- length= 7;
107- }
108-
109- if (datetime->microsecond)
110- {
111- drizzle_set_byte4(&ptr[7], datetime->microsecond);
112- length= 11;
113- }
114-
115- return ptr + length;
116+ drizzle_set_byte1(ptr, length);
117+ return ptr + 1 + length;
118 }
119
120-void drizzle_unpack_time(drizzle_field_t field, size_t length, unsigned char *data)
121+void drizzle_unpack_time(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime)
122 {
123- drizzle_datetime_st *datetime= (drizzle_datetime_st*) data;
124- memset(datetime, 0, length);
125+ memset(datetime, 0, sizeof(*datetime));
126
127 if (length)
128 {
129 datetime->negative= field[0];
130 datetime->day= drizzle_get_byte4(&field[1]);
131 datetime->hour= field[5];
132- datetime->hour= datetime->day * 24;
133- datetime->day= 0;
134 datetime->minute= field[6];
135 datetime->second= field[7];
136 if (length > 8)
137@@ -267,10 +274,9 @@
138 }
139 }
140
141-void drizzle_unpack_datetime(drizzle_field_t field, size_t length, unsigned char *data)
142+void drizzle_unpack_datetime(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime)
143 {
144- drizzle_datetime_st *datetime= (drizzle_datetime_st*) data;
145- memset(datetime, 0, length);
146+ memset(datetime, 0, sizeof(*datetime));
147
148 if (length)
149 {
150
151=== modified file 'libdrizzle/pack.h'
152--- libdrizzle/pack.h 2013-03-06 18:05:42 +0000
153+++ libdrizzle/pack.h 2013-04-01 17:45:26 +0000
154@@ -79,9 +79,9 @@
155
156 unsigned char *drizzle_pack_datetime(drizzle_datetime_st *datetime, unsigned char *ptr);
157
158-void drizzle_unpack_time(drizzle_field_t field, size_t length, unsigned char *data);
159+void drizzle_unpack_time(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime);
160
161-void drizzle_unpack_datetime(drizzle_field_t field, size_t length, unsigned char *data);
162+void drizzle_unpack_datetime(drizzle_field_t field, size_t length, drizzle_datetime_st *datetime);
163
164 /**
165 * Unpack length-encoded string.
166
167=== modified file 'libdrizzle/statement.cc'
168--- libdrizzle/statement.cc 2013-03-21 10:44:02 +0000
169+++ libdrizzle/statement.cc 2013-04-01 17:45:26 +0000
170@@ -471,13 +471,13 @@
171 break;
172 case DRIZZLE_COLUMN_TYPE_TIME:
173 param->data= param->data_buffer;
174- drizzle_unpack_time(row[column_counter], param->length, (unsigned char*)param->data);
175+ drizzle_unpack_time(row[column_counter], param->length, (drizzle_datetime_st *)param->data);
176 break;
177 case DRIZZLE_COLUMN_TYPE_DATE:
178 case DRIZZLE_COLUMN_TYPE_DATETIME:
179 case DRIZZLE_COLUMN_TYPE_TIMESTAMP:
180 param->data= param->data_buffer;
181- drizzle_unpack_datetime(row[column_counter], param->length, (unsigned char*)param->data);
182+ drizzle_unpack_datetime(row[column_counter], param->length, (drizzle_datetime_st *)param->data);
183 break;
184 case DRIZZLE_COLUMN_TYPE_TINY_BLOB:
185 case DRIZZLE_COLUMN_TYPE_MEDIUM_BLOB:
186
187=== modified file 'libdrizzle/statement_param.cc'
188--- libdrizzle/statement_param.cc 2013-03-11 17:33:00 +0000
189+++ libdrizzle/statement_param.cc 2013-04-01 17:45:26 +0000
190@@ -129,6 +129,8 @@
191 drizzle_datetime_st *time;
192 time= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
193
194+ bzero(time, sizeof(*time));
195+
196 time->negative= is_negative;
197 time->day= days;
198 time->hour= hours;
199@@ -145,6 +147,8 @@
200 drizzle_datetime_st *timestamp;
201 timestamp= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
202
203+ bzero(timestamp, sizeof(*timestamp));
204+
205 timestamp->negative= false;
206 timestamp->year= year;
207 timestamp->day= day;
208@@ -156,7 +160,7 @@
209 timestamp->microsecond= microseconds;
210
211 /* Length not important because we will figure that out when packing */
212- return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_TIME, timestamp, 0, false);
213+ return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_TIMESTAMP, timestamp, 0, false);
214 }
215
216 bool drizzle_stmt_get_is_null_from_name(drizzle_stmt_st *stmt, const char *column_name, drizzle_return_t *ret_ptr)
217@@ -619,29 +623,46 @@
218 {
219 /* Max time is -HHH:MM:SS.ssssss + NUL = 17 */
220 char* buffer= param->data_buffer + 50;
221- if (time->microsecond == 0)
222- {
223- snprintf(buffer, 17, "%s%"PRIu16":%"PRIu8":%"PRIu8, (time->negative) ? "-" : "", time->hour, time->minute, time->second);
224- }
225- else
226- {
227- snprintf(buffer, 17, "%s%"PRIu16":%"PRIu8":%"PRIu8".%"PRIu32, (time->negative) ? "-" : "", time->hour, time->minute, time->second, time->microsecond);
228- }
229+ int buffersize = 17;
230+ int used = 0;
231+
232+ /* Values are transferred with days separated from hours, but presented with days folded into hours. */
233+ used = snprintf(buffer, buffersize-used, "%s%02u:%02"PRIu8":%02"PRIu8, (time->negative) ? "-" : "", time->hour + 24 * time->day, time->minute, time->second);
234+
235+ /* 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 */
236+ if (time->microsecond)
237+ used += snprintf(buffer+used, buffersize-used, ".%06" PRIu32, time->microsecond);
238+
239+ assert(used < buffersize);
240+
241 return buffer;
242 }
243
244 char *timestamp_to_string(drizzle_bind_st *param, drizzle_datetime_st *timestamp)
245 {
246- /* Max timestamp is YYYY-MM-DD HH:MM:SS.ssssss + NUL = 26 */
247+ /* Max timestamp is YYYY-MM-DD HH:MM:SS.ssssss + NUL = 27 */
248 char* buffer= param->data_buffer + 50;
249- if (timestamp->microsecond == 0)
250- {
251- snprintf(buffer, 26, "%"PRIu16"-%"PRIu8"-%"PRIu32" %"PRIu16":%"PRIu8":%"PRIu8, timestamp->year, timestamp->month, timestamp->day, timestamp->hour, timestamp->minute, timestamp->second);
252- }
253- else
254- {
255- snprintf(buffer, 26, "%"PRIu16"-%"PRIu8"-%"PRIu32" %"PRIu16":%"PRIu8":%"PRIu8".%"PRIu32, timestamp->year, timestamp->month, timestamp->day, timestamp->hour, timestamp->minute, timestamp->second, timestamp->microsecond);
256- }
257+ int buffersize = 27;
258+ int used = 0;
259+
260+ used += snprintf(buffer, buffersize-used, "%"PRIu16"-%02"PRIu8"-%02"PRIu32,
261+ timestamp->year, timestamp->month, timestamp->day);
262+ assert(used < buffersize);
263+
264+ if (param->type == DRIZZLE_COLUMN_TYPE_DATE)
265+ return buffer;
266+
267+ used += snprintf(buffer+used, buffersize-used, " %02"PRIu16":%02"PRIu8":%02"PRIu8,
268+ timestamp->hour, timestamp->minute, timestamp->second);
269+
270+ /* 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 */
271+ if (timestamp->microsecond)
272+ {
273+ used += snprintf(buffer+used, buffersize-used, ".%06"PRIu32, timestamp->microsecond);
274+ }
275+
276+ assert(used < buffersize);
277+
278 return buffer;
279 }
280

Subscribers

People subscribed via source and target branches

to all changes: