Return TIMESTAMP columns as native Python datetime objects to improve support for Apache Superset

[Aymaru]

trafficstars

Summary of the changes / Why this is an improvement

Changing the return type from set to list on get_pk_constraint(), solves a compatibility issue from Apache Superset that doesn't show the metadata from CrateDB tables in the SQL Lab.

CrateDB types TIMESTAMP and TIMESTAMP WITHOUT TIME ZONE, are returned as python datetime objects, correctly displaying date columns in charts and SQL Lab from Apache Superset.

Checklist

• [x ] CLA is signed

[amotl]

Dear @Aymaru,

thanks a stack for your contribution. We will have a look and might come back to you with some comments.

With kind regards, Andreas.

[amotl]

Dear @Aymaru,

first of all, thank you very much for your contribution again. I was able to take a more detailed look at the implementation right now and wanted to start a discussion around your patch.

Apologies that the CI job apparently didn't work when you submitted your patch. I enabled it again (see https://github.com/crate/crate-python/pull/396#issuecomment-784370324) and just triggered it by pushing (and then removing) an empty commit to your PR.

The outcome is that the patch currently doesn't pass the tests. The spots are:

• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:40
• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:64
• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:138
• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:210
• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:348
• https://github.com/crate/crate-python/runs/2003433298?check_suite_focus=true#step:5:396

Will you be able to fix those? I also added some thoughts about the implementation itself.

With kind regards, Andreas.

[amotl]

Dear @Aymaru,

thank you very much for continuing your work on this. Please let me know if you are still able to spend time for more iterations on it or whether you would like to see us taking over. On the patch itself, I will add some more comments.

With kind regards, Andreas.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 57ec0908ea19f7601382996a2958e70fe8be33fc into 71adf4d866e73cc8c2e860ec79bf3bf3f2b8fb54 - view on LGTM.com

new alerts:

• 1 for Unguarded next in generator

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 9b82ac243b8fd1b1c01e9f271d55fc8e32c38bfc into 71adf4d866e73cc8c2e860ec79bf3bf3f2b8fb54 - view on LGTM.com

new alerts:

• 1 for Unguarded next in generator

[amotl]

Dear @Aymaru,

first things first: As I noticed recent activity from you on this patch again, I wanted to thank you for keeping up to it.

Further, I wanted to take the chance to tell you that we recently fixed some flaws within the test suite. Besides it now also works on macOS, more importantly, it received different fixes where the outcome of the software tests was indeterministic before. So, you might want to rebase your branch on top of the current master branch in order to also receive those improvements.

With kind regards, Andreas.

P.S.: Because we updated the installation procedure (i.e. removing bootstrap.py), I am also humbly asking you to reevaluate the DEVELOP.rst file in order to be fully up to speed with the recent improvements.

[amotl]

Dear @Aymaru,

the main aspects of your patch have been converged into crate/crate-python#442, crate/crate-python#445, and crate/sqlalchemy-cratedb#89.

With crate/crate-python#426, we evaluated that the specific detail of returning a list from get_pk_constraint creates a regression when used with Apache Superset, so we did not take it into further consideration. Please correct me if you think I am wrong.

With kind regards, Andreas.

[amotl]

Hi again.

@hlcianfagna: Do you think that with your recent patch to fix the CrateDB adapter in Apache Superset, this issue can be considered as resolved, or do you think it is referring to a similar but different detail on datetime/timestamp compatibility matters?

• https://github.com/apache/superset/pull/27567

@Aymaru: May we humbly ask you to check again, using the most recent versions of both Apache Superset, and CrateDB?

With kind regards, Andreas.

[amotl]

Dear @hlcianfagna and @Aymaru,

we would like to thank you once again for your excellent improvements on this matter. It looks like Hernan's recent improvements made it into the upstream codebase of Apache Superset, so we may consider this issue as resolved?

@Aymaru: Can you verify if everything you aimed at works well with CrateDB now, when using a recent version of Apache Superset?

With kind regards, Andreas.

[amotl]

Hi again. Based on those improvements, and that we didn't hear about any other flaws recently, we are closing this PR, in the assumption that everything works well in this regard.

• https://github.com/apache/superset/pull/27567
• https://github.com/crate/sqlalchemy-cratedb/pull/22
• https://github.com/crate/sqlalchemy-cratedb/pull/25
• https://github.com/apache/superset/pull/29243

Please let us know if you think anything is still wrong, best raised on the issue tracker of sqlalchemy-cratedb. Thank you again for your efforts on this!