ENH: SQLAlchemy Default precision and scale to decimal types for PostgreSQL and MySQL

[ian-r-rose]

Possible fix for #1273

Adds default precision and scale to decimal types for PostgreSQL and MySQL. Numeric data types without defined precision and scale are allowed in these dialects (even if it is recommended that you specify them), and these tables exist in the wild.

I'm not sure if this is the best fix, but I was hoping to get a conversation started.

[xmnlab]

@ian-r-rose when you have a change, could you rebase your code on top of master?

[ian-r-rose]

Rebased

[xmnlab]

it seems the tests have some conflict between float and decimal
maybe you can try sqlalchemy.types.DECIMAL

[ian-r-rose]

Really, I'm not particularly interested in MySQL right now (just Postgres), but I'm having a tough time deciding how to handle when the generic SQLAlchemy backend should be used, and when a more specific backend should be used. In particular, I'm not really sure how the multiple dispatch handles this case.

[xmnlab]

    -    float_col : float32
    +    float_col : decimal(1000, 0)

it seems the test for python 3.5 has this problem between float and decimal
maybe because the libraries in this environment is no updated.

[ian-r-rose]

Thanks for taking a look. I'm not sure what you mean here, what library is not updated?

[xmnlab]

For compatibility issues we needed to pin some libraries inside CI with python 3.5 so the environment is out-of-date .. and probably it is the reason it is raising this error El lun., 30 de septiembre de 2019 11:35, Ian Rose <notifications@github.com> escribió:

…

***@***.**** commented on this pull request. ------------------------------ In ibis/sql/alchemy.py <#1969 (comment)>: > @@ -100,6 +100,25 @@ def sa_boolean(_, satype, nullable=True): return dt.Boolean(nullable=nullable) ***@***.***(MySQLDialect, sa.types.Numeric) Thanks for taking a look. I'm not sure what you mean here, what library is not updated? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1969?email_source=notifications&email_token=ABHX5HNB2GKIBJUOAZEWX6TQMIMCPA5CNFSM4IXV5WZ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGLFTKI#discussion_r329646153>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHX5HJOOIMIB3FVN4ERP53QMIMCPANCNFSM4IXV5WZQ> .

[ian-r-rose]

Perhaps a better approach would be to allow None for precision and scale, since SQLAlchemy allows it?

[ian-r-rose]

Okay, this is now passing CI.

[xmnlab]

Looks great @ian-r-rose, thanks for working on that.
let's give one more day to see if it receives more review.
and I can merge that tomorrow.

[xmnlab]

@ian-r-rose I was checking the tests and it seems there is no much tests for decimal.
would you mind to add some tests for decimal for the 3 backends related here? (mysql, postgresql and sqlite)

you can add the test to https://github.com/ibis-project/ibis/blob/master/ibis/tests/all/test_numeric.py

let me know if you want a hand with these tests.

[ian-r-rose]

@xmnlab I can take a look, though I'm not really sure what these tests would look like. Do you have any examples to point me towards? I find the ibis test suite to be kind of confusing.

[xmnlab]

@ian-r-rose feel free to open an issue if you have any suggestion to improve the tests :) or to share your feelings about the current tests!

I think maybe just simple tests for the decimal operation with default and not default precision and scale should be enough.

there are some expression test here: https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/test_decimal.py

by expression test I mean it just test the expression it doesn't test it for the backend.

maybe you could get some tests from there and create the test for these 3 backends at /ibis/tests/all/test_numeric.py

[ian-r-rose]

It's not really clear to me how to check for different behavior among the different backends in the expression tests. That is to say, I can instantiate datatypes or expressions as per the tests in test_decimal, but I don't know how to say "I expect the default precision and scale to be different for these backends".

[ian-r-rose]

@xmnlab I've added some tests for this for PostgreSQL. I didn't see an obvious place for any mysql tests -- any suggestions? This PR doesn't touch sqlite, really.

[toryhaavik]

i think this looks good to me

[xmnlab]

Thanks @toryhaavik I am checking something. And if it is ok I will merge that.

[xmnlab]

hey @toryhaavik I need more time to review that.
I am playing with that (ian-r-rose#2) and it seems the sqlalchemy is not consistent to the default scale and precision .. and it needs to be forced as @ian-r-rose did it in the tests.

I need more time to review

[ian-r-rose]

Now updated with some additional testing thanks to @xmnlab

[xmnlab]

Thanks @ian-r-rose! I will open an issue about the default precision and scale for ibis expressions

[ian-r-rose]

Thanks for the review and help with the tests @xmnlab!