Timestamps are marked unsigned

[paurkedal]

When selecting from a timestamp column, the result is marked unsigned, triggering failwith "unexpected unsigned type" in Field.convert_unsigned. To reproduce it, create a table:

create table test (t timestamp);
insert into test values (now());

and run

OCAML_MARIADB_QUERY="SELECT ?, t FROM test" _build/examples/blocking/blocking_example.native

This would solve this case at least,

diff --git a/lib/field.ml b/lib/field.ml
index 73eca28..cbb0488 100644
--- a/lib/field.ml
+++ b/lib/field.ml
@@ -90,6 +90,7 @@ let convert_unsigned field = function
   | `Short -> `Int (Unsigned.UInt.to_int (cast_to uint field))
   | `Int24 | `Long -> `Int (Unsigned.UInt32.to_int (cast_to uint32_t field))
   | `Long_long -> `Int (Unsigned.UInt64.to_int (cast_to uint64_t field))
+  | `Timestamp as kind -> `Time (to_time field kind)
   | _ -> failwith "unexpected unsigned type"

 let value field =

though maybe it would be better to dispatch on both datatype and signedness in one, or datatype optionally followed by signedness.

[andrenth]

It seems that timestamps are always marked as unsigned on the server (in sql/field.cc):

Field_timestamp::Field_timestamp(...)
...
{
    /* For 4.0 MYD and 4.0 InnoDB compatibility */
    flags|= UNSIGNED_FLAG;
    ....

And in the same file, in the Create_field::check method:

  case MYSQL_TYPE_TIMESTAMP:
  case MYSQL_TYPE_TIMESTAMP2:
    ...
    flags|= UNSIGNED_FLAG;
    break;

which is not done for other date types. So it seems that simply moving the Timestamp case to convert_unsigned would be safe.

However, just skimming through that file I noticed that MYSQL_TYPE_BIT seems to always be set to unsigned too... And since this seems to only be documented in code, I think I'll go with your idea of dispatching on type and signedness.

[andrenth]

Commit c0a2c43 makes that change.

[andrenth]

I'll try to make a new release by the end of the week.

Thanks for your work in finding and fixing these bugs. I haven't had a chance to use the library in production yet as other things got in the way, so you're saving me a lot of time here :)

[paurkedal]

Great, I expect to follow up with a caqti release with mariadb support shortly after.

[andrenth]

I've just submitted 0.8.0 to the opam repo. Should be available soon.