Fix pdo type of column type NUMERIC

[mringler]

Issue

The column type NUMERIC, Propel's fixed precision number type (DECIMAL) correctly uses string in PHP, but binds parameter to PDO statments as int, which cuts off the fractional part.

Changes

PDO type is changed to string, matching DECIMAL type.

Implementation Details

All vendors use NUMERIC as alias for DECIMAL (or the corresponding type on that platform), so using a different PDO type between NUMERIC and DECIMAL has to be a bug.

Test strategy

Added simple read and write test for NUMERIC and DECIMAL.
Also added a DECIMAL column to the bookstore schema, to have at least one of them linted and analyzed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[kasparsatke]

Thanks a lot!

I now remember I have had such problems with NUMERIC in the past and avoided it subsequently. On the other hand, I think I have also tried with DECIMAL and with both types I haven't been able to handle a simple two decimal price tag in Propel, so I had to resort to FLOAT, giving up precision math, which would have come with NUMERIC/DECIMAL.

[mringler]

Oh, interesting. Do you remember what wasn't working with DECIMAL?

If there is an issue with it, NUMERIC now probably has it too...

I added simple read/write tests, which run through. Are there more complex tests we can/should add?

[kasparsatke]

It's been very long since I have dealt with the matter. I have tried to pinpoint this down through digging in both my memory and my git history. Additionally Google and the Propel/Perpl code.

It is a story of multiple things gone wrong - but I think I have both learned something again and also have an improvement suggestion for Perpl.

It begins with me thinking until yesterday that NUMERIC and DECIMAL were different. Probably I have mixed up SQL types with PHP types in my mind somehow. Or it just was the bug you just have fixed.

In my history, I could only retrieve the point ca. 4.5 years ago when I have changed my column definition from type="decimal" size="10, 2" to type="float" size="10, 2". I have then tried to find out again whether that was some hack with the space in between or it just suddenly worked because float accepts decimals out of the box. I could not find the reason for the space but found out that Propel/Perpl just doesn't work with a normal size definition for NUMERIC/DECIMAL, as outlined here: https://dev.mysql.com/doc/refman/8.4/en/fixed-point-types.html.

Instead, I would have needed to define my column type as follows:

type="decimal" size="10" scale="2"

Because until now, Propel/Perpl always int-casts the size attribute. But then again, someone tried it with scale and failed so as well in here: propelorm/PropelBundle#320. Marc's answer here doesn't satisfy me as well, as yes the default for size in MySQL is 10 but there is no default for scale.

Regardless if the scale attribute works or not - first of all I would have never expected that this is needed at all and secondly I find it ill-conceived to put the scale into a separate attribute as a normal SQL column definition would just be DECIMAL(10,2).

So why don't we allow size attributes just to include the scale value?

P.S.:
I have found the reason for the space in between. Sometimes the size is documented as [Precision, Scale], sometimes [Precision,Scale], but to me it is not clear if both are accepted across RDBMSs or not. MSSQL e.g. uses both formats here.

[mringler]

Ah, I see, thank you for researching!

I get confused with NUMERIC and DECIMAL too. Had to google it, there is a good explanation on Stack Overflow - and I'll probably forget it shorty.

The space ahead of "Scale" in [Precision, Scale]/[Precision, Scale] does not matter. I think you can put as many as you like, Decimal( 12 , 4 ) should work just the same.

In regards to the size/scale attributes, I think the idea is to see them as unified named argument, i.e. a size variable that will be set into CHAR($size), TIMESTAMP($size), DECIMAL($size), etc.
I'm torn on supporting scale as part of size (size="10,2").
On one hand, I can never remember that it is called scale and have to look it up each time, so that sucks, and changing it is not much work, and overall, from your description, the whole setup sounds non-intuitive (and improperly documented?).
On the other hand, using an inner format in an attribute does not sit right with me. Because in theory, we could put the whole column definition in a csv-style string and set it on the xml tag in a single attribute, but that is obviously not a good idea - why would it be different on a smaller scale?

The linked post, is that true, does Propel/Perpl omit precision and scale if they are the default values? That would mean that someone deliberately added the default values to the code, implemented a comparison and an adjustment to the output when the values match. A lot of work to make the output less verbose.
I'd be happy to remove that if it is still there.

[kasparsatke]

On one hand, I can never remember that it is called scale and have to look it up each time, so that sucks, and changing it is not much work, and overall, from your description, the whole setup sounds non-intuitive (and improperly documented?).

Fully agree

On the other hand, using an inner format in an attribute does not sit right with me.

I would not call it an inner format - at least for us. Because semantically, "our" size attribute is also uniformly used across data types in SQL, be it CHAR(4) and VARCHAR(4), INT(4), DECIMAL(4), DECIMAL(4,2), FLOAT(4,2) or TIME(4). Although they can mean different things, MySQL just calls all of this attribute the data type description. To my understanding from this, the scale part intrinsically belongs to the precision part. It is just not specified/needed everywhere.

"Our" size attribute already means a lot of different things, depending on the byte hunger of the actual data type. So if this is an inner format - it would actually be on the SQL side and we would just be mirroring it. So I would argue in favor of a "magical" size attribute and just see it as an elongated arm of the data type description from SQL and hence also let it be just as magical as it is in SQL.

The linked post, is that true, does Propel/Perpl omit precision and scale if they are the default values? That would mean that someone deliberately added the default values to the code, implemented a comparison and an adjustment to the output when the values match. A lot of work to make the output less verbose.
I'd be happy to remove that if it is still there.

Actually I could not verify this so far. And it does not make sense with the linked post and Marc's answer. Because although 10 Precision is default on SQL side, it should not be default on Propel/Perpl side and why should Propel/Perpl even care about that? A default would only matter if size would be omitted but then it would not be on Propel/Perpl to figure out but on the RDBMS side. And with regards to the scale attribute it makes even less sense as there is not even a default on SQL side (for obvious reason, as scale is optional and only really matters on DOUBLE/FLOAT, DECIMAL/NUMERAL).

The only default size in Propel/Perpl I have found so far is 255 for VARCHAR without size value.