enable Decimal support for Mysql #1019

Fiedzia · 2017-07-12T05:34:27Z

Whole decimal handling was really confusing, so I have no idea if I got this right, but seems to work.
I am still looking on how to write tests for it.

killercup · 2017-07-12T07:21:11Z

cc @rubdos who added BigDecimal support for Postgres

sgrif · 2017-07-12T19:37:47Z

diesel/src/mysql/types/numeric.rs

+    impl ToSql<types::Numeric, Mysql> for BigDecimal {
+        fn to_sql<W: Write>(&self, out: &mut W) -> Result<IsNull, Box<Error + Send + Sync>> {
+            // Mysql representats decimal type as char[]
+            out.write(&format!("{}", *self).into_bytes()).map(|_| IsNull::No).map_err(|e| e.into())


How about write!(out, "{}", self)

sgrif · 2017-07-12T19:39:55Z

diesel/src/mysql/types/numeric.rs

+pub mod bigdecimal {
+    extern crate num_traits;
+    extern crate num_bigint;
+    extern crate num_integer;


Do we need any of these besides bigdecimal?

Eijebong

Can you add some tests ?
Also some documentation about the fact that MySQL supports the BigDecimal type would be great

Eijebong · 2017-07-12T19:44:41Z

diesel/src/mysql/types/mod.rs


 use byteorder::WriteBytesExt;
-use mysql::{Mysql, MysqlType};
+pub use mysql::{Mysql, MysqlType};


Why is this needed ?

Eijebong · 2017-07-12T19:45:03Z

diesel/src/mysql/types/mod.rs

 use std::error::Error as StdError;
 use std::io::Write;
 use types::{ToSql, IsNull, FromSql, HasSqlType};

+pub mod sql_types {


We don't reexport BigDecimal for postgres, why should we for MySQL ?

I think you can remove the whole sql_types module :)

Eijebong · 2017-07-12T19:45:32Z

diesel/src/mysql/types/mod.rs

@@ -58,3 +65,18 @@ impl HasSqlType<::types::Timestamp> for Mysql {
        MysqlType::Timestamp
    }
 }
+


Eijebong · 2017-07-12T19:45:37Z

diesel/src/mysql/types/mod.rs

+        MysqlType::String
+    }
+}
+


Eijebong · 2017-07-12T19:46:15Z

diesel/src/mysql/types/numeric.rs

+
+    use mysql::Mysql;
+
+    pub use self::bigdecimal::BigDecimal;


Why does this need to be pub use ?

Eijebong · 2017-07-12T19:46:25Z

diesel/src/mysql/types/numeric.rs

+    pub use self::bigdecimal::BigDecimal;
+
+    use types::{self, FromSql, ToSql, IsNull};
+


Fiedzia · 2017-07-13T04:54:25Z

I've updated docs and I am not reexporting BigDecimal anymore. I'll will still need to add tests.

sgrif · 2017-07-13T20:54:49Z

diesel/src/mysql/types/numeric.rs

+
+    impl ToSql<types::Numeric, Mysql> for BigDecimal {
+        fn to_sql<W: Write>(&self, out: &mut W) -> Result<IsNull, Box<Error + Send + Sync>> {
+            // Mysql representats decimal type as char[]


I don't think this comment is adding any value.

sgrif · 2017-07-13T20:54:57Z

diesel/src/mysql/types/numeric.rs

+
+    impl FromSql<types::Numeric, Mysql> for BigDecimal {
+        fn from_sql(bytes: Option<&[u8]>) -> Result<Self, Box<Error+Send+Sync>> {
+            //_ numeric


I don't think this comment is adding any value.

sgrif · 2017-07-13T20:57:53Z

diesel/src/mysql/types/numeric.rs

+    impl FromSql<types::Numeric, Mysql> for BigDecimal {
+        fn from_sql(bytes: Option<&[u8]>) -> Result<Self, Box<Error+Send+Sync>> {
+            //_ numeric
+            match str::from_utf8(not_none!(bytes)) {


Why do we need to do string conversion here? How about just BigDecimal::parse_bytes?

Fiedzia · 2017-07-14T05:34:02Z

I've enabled pg tests for mysql

killercup · 2017-07-18T12:35:00Z

@Fiedzia CI fails with

error[E0119]: conflicting implementations of trait `query_builder::query_id::QueryId` for type `types::Numeric`:
   --> /home/travis/build/diesel-rs/diesel/diesel/src/types/impls/mod.rs:160:9

Can you have a look at this?

Eijebong · 2017-07-18T18:57:13Z

Oh, you also need to rebase, it should be easy, you have to change the mysql metadata function so it takes _: &()

sgrif · 2017-07-18T19:37:13Z

Looks like something went wrong with the rebase (or with Github), this is showing all of #996 as part of this PR's changed files.

Eijebong · 2017-07-18T19:58:31Z

Hmm this is a git usage mistake. The first different commit is d9778d4 which is
9f4e9d3 in this PR.

I don't know why, but the parent commit isn't the same.

@Fiedzia How did you rebase ? :p

Fiedzia · 2017-07-18T20:03:03Z

Its me. I did git pull --rebase upstream master

Eijebong · 2017-07-18T20:15:04Z

Mhhh weird that should have worked.

Did you have any conflict ? Did you resolve then properly ?

Fiedzia · 2017-07-18T20:22:41Z

I had conflicts and I resolved them with no particular issues.

Fiedzia · 2017-07-18T21:06:23Z

I believe I resolved all issues, builds are failing due to problem with rust nightly:
rust-lang/rust#43153

sgrif · 2017-07-31T23:14:12Z

r? @Eijebong

Eijebong

I think we're good to go minus those two comments nits :)

Eijebong · 2017-08-01T07:11:59Z

diesel_tests/tests/types.rs

+        "1.0",
+        "141.0",
+        "-1.0",
+        // Powers of 10k (numeric is represented in base 10k)


This is not true for MySQL IIRC

Eijebong · 2017-08-01T07:12:44Z

diesel_tests/tests/types.rs

+
+    // Some non standard values:
+    let query = "cast(18446744073709551616 as decimal)"; // 2^64; doesn't fit in u64
+    //it is mysql, it will trim it even in strict mode


Nit: "// It is"

Eijebong · 2017-08-03T14:18:40Z

Took the liberty to rebase and add a changelog entry, I'll merge once tests are green.

Fiedzia · 2017-08-03T15:30:00Z

Linking the original issue here: #830

Eijebong · 2017-08-03T15:57:27Z

Thanks !

sgrif reviewed Jul 12, 2017

View reviewed changes

Eijebong requested changes Jul 12, 2017

View reviewed changes

sgrif reviewed Jul 13, 2017

View reviewed changes

Fiedzia force-pushed the mysql_decimal_type branch from 7f36814 to fed235a Compare July 18, 2017 21:39

Fiedzia force-pushed the mysql_decimal_type branch from 7898290 to c202870 Compare July 31, 2017 09:05

Eijebong approved these changes Aug 1, 2017

View reviewed changes

Add Decimal support for MySQL

8b069a1

Eijebong force-pushed the mysql_decimal_type branch from 4daff32 to 8b069a1 Compare August 3, 2017 14:18

Eijebong merged commit 93de941 into diesel-rs:master Aug 3, 2017

Fiedzia deleted the mysql_decimal_type branch August 3, 2017 16:04

rubdos mentioned this pull request Aug 5, 2017

No support for some MySQL data types #830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable Decimal support for Mysql #1019

enable Decimal support for Mysql #1019

Fiedzia commented Jul 12, 2017

killercup commented Jul 12, 2017

sgrif Jul 12, 2017

sgrif Jul 12, 2017

Eijebong left a comment

Eijebong Jul 12, 2017

Eijebong Jul 12, 2017

Eijebong Jul 12, 2017

Eijebong Jul 12, 2017

Eijebong Jul 12, 2017

Eijebong Jul 12, 2017

Fiedzia commented Jul 13, 2017

sgrif Jul 13, 2017

sgrif Jul 13, 2017

sgrif Jul 13, 2017

Fiedzia commented Jul 14, 2017

killercup commented Jul 18, 2017

Eijebong commented Jul 18, 2017

sgrif commented Jul 18, 2017

Eijebong commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

Eijebong commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

sgrif commented Jul 31, 2017

Eijebong left a comment

Eijebong Aug 1, 2017

Eijebong Aug 1, 2017

Eijebong commented Aug 3, 2017

Fiedzia commented Aug 3, 2017

Eijebong commented Aug 3, 2017

		pub use self::bigdecimal::BigDecimal;

		use types::{self, FromSql, ToSql, IsNull};

enable Decimal support for Mysql #1019

enable Decimal support for Mysql #1019

Conversation

Fiedzia commented Jul 12, 2017

killercup commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eijebong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fiedzia commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fiedzia commented Jul 14, 2017

killercup commented Jul 18, 2017

Eijebong commented Jul 18, 2017

sgrif commented Jul 18, 2017

Eijebong commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

Eijebong commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

Fiedzia commented Jul 18, 2017

sgrif commented Jul 31, 2017

Eijebong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eijebong commented Aug 3, 2017

Fiedzia commented Aug 3, 2017

Eijebong commented Aug 3, 2017