Add a generic data type converter to the `Cursor` object

[amotl]

trafficstars

Hi there,

Introduction

After converging #395 to #437, it became clear / emerged that we wanted to have the type conversion

• only being optionally enabled with a feature flag /cc @mfussenegger
• being implemented more elegantly /cc @matriv

This patch aims to resolve both aspects.

Details

This will allow converting fetched data from CrateDB data types to Python data types in different ways. Its usage is completely optional. When not used, the feature will not incur any overhead.

There is also a DefaultTypeConverter, which aims to become a sane default choice when looking at this kind of convenience. It will enable the user to work with native Python data types from the start, for all database types where this makes sense, without needing to establish the required set of default value converters on their own.

If the DefaultTypeConverter does not fit the user's aims, it is easy to define custom type converters, possibly reusing specific ones from the library.

Thoughts

In the form of an early review, please let me know if you endorse this approach and which adjustments you would like to see.

Personally, I would add more test cases to test_cursor.py, in order to exercise the machinery in more detail and to increase code coverage, and probably make the functionality more present in the documentation.

Also, as @mfussenegger mentioned at https://github.com/crate/crate-python/pull/437/files#r930822457, it might make sense to also add the important aspect of converting elements within ARRAY types to this patch already.

With kind regards, Andreas.

[seut]

👍 I really like this approach and would definitely prefer it over #437.

[mfussenegger]

There is btw. also https://peps.python.org/pep-0249/#type-objects in the DB API definition - I didn't have a closer look at it, but we should double check to see if it affects what the PR would be doing.

[amotl]

Thank you for endorsing this patch and for your valuable suggestions.

[amotl]

Hi again,

other than the inline comments, let me address further questions here.

Personally, I would add more test cases to test_cursor.py, in order to exercise the machinery in more detail and to increase code coverage.

Done, mostly with 141054780f.

It might make sense to also add the important aspect of converting elements within ARRAY types to this patch already.

Done with b21c38e99.

I'm also not sure about the level of separation. E.g. the converter application could probably be separated from the cursor without much loss of API/UI convenience. But no strong opinion here.

I also thought back and forth about this detail. In the end, I followed the path outlined by https://github.com/laughingman7743/PyAthena very closely, where I originally discovered this way of doing it. See PyAthena/converter.py.

On the other hand, I think something common like setting the timezone for datetime objects could be made even easier for users.

Indeed, that would be sweet. I will think about how we could add this feature.

There is btw. also https://peps.python.org/pep-0249/#type-objects in the DB API definition - I didn't have a closer look at it, but we should double check to see if it affects what the PR would be doing.

I also found that section in the DBAPI spec, but I think it is exclusively about input parameter conversion:

Many databases need to have the input in a particular format for binding to an operation’s input parameters. For example, if an input is destined for a DATE column, then it must be bound to the database in a particular string format.

On the other hand, the current implementation is exclusively about output data conversion. That being said, the converter functions itself can well be reused for that use case, but otherwise I think it is a different area, and to be addressed within a different patch.

With kind regards, Andreas.

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging 1dc87ee5bc88f56166583e549660ddbe8c3621d9 into b21f1791ea6fcc80a3deb79728a74ac05de05903 - view on LGTM.com

new alerts:

• 2 for Unused import