implement f32 data type #1145

pythonbrad · 2023-04-10T15:24:12Z

Implementation of f32 for issue #623

coveralls · 2023-04-10T17:25:36Z

Pull Request Test Coverage Report for Build 4802818313

1071 of 1079 (99.26%) changed or added relevant lines in 14 files are covered.
8 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 98.987%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core/src/data/value/literal.rs	46	49	93.88%
core/src/executor/evaluate/function.rs	0	5	0.0%

Files with Coverage Reduction	New Missed Lines	%
core/src/executor/evaluate/function.rs	1	97.78%
core/src/executor/insert.rs	7	96.34%

Totals
Change from base Build 4777188706:	0.2%
Covered Lines:	43072
Relevant Lines:	43513

💛 - Coveralls

ChobobDev · 2023-04-11T15:45:47Z

I can't wait to see another data type being added to our project :)
Thanks in advance

pythonbrad · 2023-04-13T12:21:27Z

@panarch , can i add tests not related to the f32 type but to other types?

panarch · 2023-04-13T16:34:58Z

@panarch , can i add tests not related to the f32 type but to other types?

Sure! but it would be good to make a separate PR, then I can do quick review and merge.

pythonbrad · 2023-04-13T16:59:13Z

Ok, I think so

ever0de · 2023-04-23T05:20:24Z

core/src/data/value/convert.rs

+            Value::I32(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
+            Value::I64(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,


Suggested change

Value::I32(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,

Value::I64(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,

Value::I32(value) => *value as f32,

Value::I64(value) => *value as f32,

Could you please also change the f64 below? These types have a smaller range than f32.

ever0de · 2023-04-23T05:20:31Z

core/src/data/value/convert.rs

+            Value::U8(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
+            Value::U16(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
+            Value::U32(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
+            Value::U64(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
+            Value::U128(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,


ever0de · 2023-04-23T05:21:58Z

core/src/data/value/convert.rs

@@ -872,6 +944,8 @@ mod tests {
        test!(Value::U32(122), Ok(122));
        test!(Value::U64(122), Ok(122));
        test!(Value::U128(122), Ok(122));
+        test!(Value::F32(122.0_f32), Ok(122));
+        test!(Value::F64(122.9), Ok(122));


Suggested change

test!(Value::F64(122.9), Ok(122));

There is duplicate code below. Could you please remove it?

ever0de · 2023-04-23T05:36:15Z

core/src/data/value/mod.rs

            (F64(a), _) => a.try_multiply(other),
            (Decimal(a), _) => a.try_multiply(other),
            (Interval(a), I8(b)) => Ok(Interval(*a * *b)),
            (Interval(a), I16(b)) => Ok(Interval(*a * *b)),
            (Interval(a), I32(b)) => Ok(Interval(*a * *b)),
            (Interval(a), I64(b)) => Ok(Interval(*a * *b)),
            (Interval(a), I128(b)) => Ok(Interval(*a * *b)),
+            (Interval(a), F32(b)) => Ok(Interval(*a * (*b) as f64)),


Is there a specific reason for casting? I think performing the operation with multiplication in f32 would be desirable.

ever0de · 2023-04-23T05:37:37Z

core/src/data/value/mod.rs

@@ -478,6 +494,7 @@ impl Value {
            (Interval(a), U32(b)) => Ok(Interval(*a / *b)),
            (Interval(a), U64(b)) => Ok(Interval(*a / *b)),
            (Interval(a), U128(b)) => Ok(Interval(*a / *b)),
+            (Interval(a), F32(b)) => Ok(Interval(*a / (*b as f64))),


ever0de · 2023-04-23T05:49:52Z

core/src/data/value/mod.rs

+        test!(add F32(1.0_f32), F64(2.0) => F64(3.0));
+        test!(add F32(1.0_f32), I8(2)    => F32(3.0_f32));
+        test!(add F32(1.0_f32), I32(2)   => F32(3.0_f32));
+        test!(add F32(1.0_f32), I64(2)   => F32(3.0_f32));


Some data types are empty (there are also other places that need to be organized, so it requires a cleanup. The code for organizing other places might be better handled in a separate PR).
ex) U8, U16, ... ?

ever0de · 2023-04-23T05:52:31Z

core/src/data/value/mod.rs

+        test!(subtract I8(3),    F32(2.0_f32) => F32(1.0_f32));
+        test!(subtract I32(3),   F32(2.0_f32) => F32(1.0_f32));
+        test!(subtract I64(3),   F32(2.0_f32) => F32(1.0_f32));
+        test!(subtract I128(3),  F32(2.0_f32) => F32(1.0_f32));


ever0de · 2023-04-23T06:14:26Z

test-suite/src/function/extract.rs

This is not related to this PR. Please submit it as a separate PR or open an issue for it.

ever0de · 2023-04-23T06:16:51Z

core/src/data/value/mod.rs

+        assert_eq!(U32(9).sqrt(), Ok(F32(3.0_f32)));
+        assert_eq!(U64(9).sqrt(), Ok(F32(3.0_f32)));
+        assert_eq!(U128(9).sqrt(), Ok(F32(3.0_f32)));
+        assert_eq!(F32(9.0_f32).sqrt(), Ok(F32(3.0_f32)));


It seems that there are no cases for (f64, f32) and (f32, f64).

panarch

Looks all nice!!
Just before the merge, could you add an integration test of float32 in test-suite?

ref.
https://github.com/gluesql/gluesql/tree/main/test-suite/src/data_type

panarch

Looks all great!!
Thanks a lot 👍

pythonbrad marked this pull request as ready for review April 10, 2023 17:28

panarch requested review from panarch, ever0de and devgony April 11, 2023 04:46

panarch assigned pythonbrad Apr 11, 2023

panarch added the enhancement New feature or request label Apr 11, 2023

pythonbrad marked this pull request as draft April 13, 2023 11:56

pythonbrad marked this pull request as ready for review April 14, 2023 12:02

ever0de reviewed Apr 23, 2023

View reviewed changes

pythonbrad force-pushed the f32 branch 2 times, most recently from 3d9504b to b94f3f5 Compare April 24, 2023 17:14

implement f32 data type

294d067

pythonbrad force-pushed the f32 branch from b94f3f5 to 294d067 Compare April 24, 2023 18:08

panarch requested changes Apr 25, 2023

View reviewed changes

panarch requested a review from ever0de April 25, 2023 13:52

pythonbrad added 2 commits April 25, 2023 18:39

apply reviews

35bb961

apply fmt

058396b

panarch self-requested a review April 26, 2023 14:49

panarch approved these changes Apr 26, 2023

View reviewed changes

panarch merged commit 05f717c into gluesql:main Apr 26, 2023
9 checks passed

pythonbrad deleted the f32 branch April 27, 2023 05:35

This was referenced Apr 29, 2023

Add Numeric Data Type in core workspace #623

Closed

implement f32 #688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement f32 data type #1145

implement f32 data type #1145

pythonbrad commented Apr 10, 2023 •

edited

coveralls commented Apr 10, 2023 •

edited

ChobobDev commented Apr 11, 2023

pythonbrad commented Apr 13, 2023

panarch commented Apr 13, 2023

pythonbrad commented Apr 13, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

ever0de Apr 23, 2023

panarch left a comment

panarch left a comment

		Value::I32(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,
		Value::I64(value) => value.to_f32().ok_or(ValueError::ImpossibleCast)?,

implement f32 data type #1145

implement f32 data type #1145

Conversation

pythonbrad commented Apr 10, 2023 • edited

coveralls commented Apr 10, 2023 • edited

Pull Request Test Coverage Report for Build 4802818313

💛 - Coveralls

ChobobDev commented Apr 11, 2023

pythonbrad commented Apr 13, 2023

panarch commented Apr 13, 2023

pythonbrad commented Apr 13, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panarch left a comment

Choose a reason for hiding this comment

panarch left a comment

Choose a reason for hiding this comment

pythonbrad commented Apr 10, 2023 •

edited

coveralls commented Apr 10, 2023 •

edited