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

Aymaru · 2021-02-22T16:36:59Z

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

This is to fix the apache superset bug, that doesn't show the cratedb table metadata in the sqleditor

This returns the data type of the columns in a query, to allow us to transform the dates from cratedb (long int) to python datetime

In execute(), transform the columns with type timestamp and timestamp without time zone to python datetime, this will correctly display dates in apache superset

amotl · 2021-02-23T17:37:06Z

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 · 2021-03-01T11:47:09Z

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 #396 (comment)) 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:

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

With kind regards,
Andreas.

amotl · 2021-03-01T12:15:28Z

src/crate/client/cursor.py

+                col_types = self._result["col_types"]
+                tmp_data = self._result["rows"]
+
+                rows_to_convert = self._get_rows_to_convert_to_date(col_types)
+                tmp_data = self._convert_dates_to_datetime(tmp_data, rows_to_convert)
+
+                self._result["rows"] = tmp_data
+
            self.rows = iter(self._result["rows"])


Maybe we can come up with a more elegant and efficient implementation. When trying to follow the code, I recognize that it builds up a list of boolean flags which designates which columns to apply the datetime conversion to, right?

Based on that information, the code will then apply the conversion function on each column while mapping each row into memory through the list() function.

I am a bit hesitant on this and wanted to share my specific concerns:

a) The code might allocate a significant amount of memory on larger results.
b) On a very large result set, it might even run out of memory.
c) When applying the conversion, there are currently no sanity checks around which test for NULL- or other emptyness. I am specifically looking at float(str(x)[0:10]) here. Also, there is no other exception handling either. We might want to improve on this aspect.

Maybe we can wrap that conversion into a generator function in order to keep the memory footprint low and, while being at it, add some more bells and whistles on the error handling side?

amotl · 2021-03-15T16:11:12Z

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.

amotl · 2021-03-15T16:14:45Z

src/crate/client/cursor.py

+                rows_to_convert = self._get_rows_to_convert_to_date(self._result["col_types"])
+                for flag in rows_to_convert:
+                    if flag:
+                        t_rows = (row for row in self._result["rows"])


Line 61 will probably still load the full result into memory, right?

I was thinking about swapping in the generator right in between of self.rows = iter(self._result["rows"]), which is then responsible for converting between data types (timestamp epoch integer to Python datetime object).

In that manner, the pre-computation using _get_rows_to_convert_to_date() might become obsolete.

amotl · 2021-03-15T16:16:20Z

src/crate/client/sqlalchemy/dialect.py

-                return set(map(lambda el: el[0], rows))
+                return list(set(map(lambda el: el[0], rows)))


While I don't know about the specific background why there has been a set used here, can you elaborate why this should now return a list?

Dear @Aymaru,

apologies for the late reply. We've split off this amendment into #426. For you, it apparently solved this issue:

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.

With the patch at #426, we are observing a regression. Maybe due to the upgrade to SQLAlchemy 1.4, and maybe other changes in Apache Superset, this is not needed anymore?

With kind regards,
Andreas.

amotl · 2021-03-15T16:21:33Z

src/crate/client/doctests/client.txt

-     1373932800000,
+     datetime.datetime(2013, 7, 15, 18, 0),


This currently raises an exception on CI, see [1]. The current code expects datetime.datetime(2013, 7, 16, 0, 0) here.

[1] https://github.com/crate/crate-python/runs/2114206441?check_suite_focus=true#step:5:388

lgtm-com · 2021-03-30T18:49:12Z

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

new alerts:

1 for Unguarded next in generator

lgtm-com · 2021-03-30T19:09:18Z

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

new alerts:

1 for Unguarded next in generator

amotl · 2021-03-31T18:18:01Z

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.

- Adjust and add some badges to README.rst. - For covering the most recent SQLAlchemy 1.3 release, bump to SQLAlchemy version 1.3.23. - Also, enable listening to "workflow_dispatch" events in order to be able to trigger workflows manually from the UI.

…ormat

amotl · 2022-09-29T18:01:51Z

Dear @Aymaru,

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

With #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 · 2024-05-08T23:06:12Z

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?

fix(db_engine_specs): Update convert_dttm to work correctly with CrateDB apache/superset#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 · 2024-06-17T20:06:23Z

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 · 2024-06-26T15:57:00Z

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.

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!

Aymaru added 3 commits February 22, 2021 09:57

return a list instead of a set on get_pk_constraint

0212487

This is to fix the apache superset bug, that doesn't show the cratedb table metadata in the sqleditor

Changed the sql_path

f9bd08c

This returns the data type of the columns in a query, to allow us to transform the dates from cratedb (long int) to python datetime

Transform dates from crate to python datetime

66e7b51

In execute(), transform the columns with type timestamp and timestamp without time zone to python datetime, this will correctly display dates in apache superset

amotl mentioned this pull request Feb 23, 2021

Adjust badges, bump SQLAlchemy test version and add "workflow_dispatch" #396

Merged

amotl mentioned this pull request Feb 26, 2021

SqlAlchemy 1.3 Support for Apache Superset 1.0.1 crate/crate#11072

Closed

amotl force-pushed the datetime-format branch from c3cd76f to 66e7b51 Compare March 1, 2021 11:39

amotl reviewed Mar 1, 2021

View reviewed changes

amotl changed the title ~~Datetime format~~ Return TIMESTAMP columns as native Python datetime objects to improve support for Apache Superset Mar 1, 2021

Aymaru and others added 11 commits March 2, 2021 15:53

Datetime conversion implemented using map and generator

3dc5cb7

Updated tests

8dfcf80

Using generators to work with large datasets

b08cb16

changed tests

efe8bfb

fix

2d1f41f

test

246d917

updated datetime transformation using generators and updated test cases

94b3d1e

cleaning debug prints

85b7c7f

Passing a generator of the flags instead of passing the list of values

2cf32e2

Removed tests

fbef448

Merge branch 'master' into datetime-format

3b66626

amotl reviewed Mar 15, 2021

View reviewed changes

updated conversion of timestamps

57ec090

Added pandas dependency

9b82ac2

Aymaru added 3 commits March 30, 2021 16:00

Changed pandas timestamp to python datetime && deleted pandas dependecy

5b4d5ec

fixed - E226 missing whitespace around arithmetic operator

e9788ea

Changed yield value

eebd670

amotl and others added 9 commits March 31, 2021 12:33

updated conversion of timestamps

e3b8d50

Added pandas dependency

32a4826

Changed pandas timestamp to python datetime && deleted pandas dependecy

2c9e3aa

fixed - E226 missing whitespace around arithmetic operator

2adf833

Changed yield value

9bc5f51

Merge branch 'master' into datetime-format

8e53da5

Merge remote-tracking branch 'origin/datetime-format' into datetime-f…

5ddced3

…ormat

Validate date type in processors

aaaf7ee

mfussenegger assigned amotl Oct 5, 2021

amotl mentioned this pull request Jun 17, 2022

Improve compatibility with Apache Superset by showing CrateDB table metadata in the SQL Lab editor #426

Closed

amotl mentioned this pull request Jul 18, 2022

Followup: Return TIMESTAMP columns as native Python datetime objects #437

Closed

2 tasks

amotl mentioned this pull request Aug 5, 2022

Add a generic data type converter to the Cursor object #442

Merged

3 tasks

amotl mentioned this pull request Sep 16, 2022

Return TIMESTAMP types as timezone-aware native Python datetime objects #445

Merged

amotl mentioned this pull request Sep 28, 2022

Enable the DefaultTypeConverter using an SQLAlchemy DBURI connection parameter crate/sqlalchemy-cratedb#89

Open

amotl marked this pull request as draft October 18, 2022 19:41

amotl closed this Jun 26, 2024

amotl requested a review from hlcianfagna June 26, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Aymaru commented Feb 22, 2021 •

edited by amotl

Loading

amotl commented Feb 23, 2021

amotl commented Mar 1, 2021 •

edited

Loading

amotl Mar 1, 2021

amotl commented Mar 15, 2021 •

edited

Loading

amotl Mar 15, 2021 •

edited

Loading

amotl Mar 15, 2021

amotl Jun 17, 2022 •

edited

Loading

amotl Mar 15, 2021

lgtm-com bot commented Mar 30, 2021

lgtm-com bot commented Mar 30, 2021

amotl commented Mar 31, 2021 •

edited

Loading

amotl commented Sep 29, 2022 •

edited

Loading

amotl commented May 8, 2024 •

edited

Loading

amotl commented Jun 17, 2024

amotl commented Jun 26, 2024

		return set(map(lambda el: el[0], rows))
		return list(set(map(lambda el: el[0], rows)))

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

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

Conversation

Aymaru commented Feb 22, 2021 • edited by amotl Loading

Summary of the changes / Why this is an improvement

Checklist

amotl commented Feb 23, 2021

amotl commented Mar 1, 2021 • edited Loading

amotl Mar 1, 2021

Choose a reason for hiding this comment

amotl commented Mar 15, 2021 • edited Loading

amotl Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

amotl Mar 15, 2021

Choose a reason for hiding this comment

amotl Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

amotl Mar 15, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Mar 30, 2021

lgtm-com bot commented Mar 30, 2021

amotl commented Mar 31, 2021 • edited Loading

amotl commented Sep 29, 2022 • edited Loading

amotl commented May 8, 2024 • edited Loading

amotl commented Jun 17, 2024

amotl commented Jun 26, 2024

Aymaru commented Feb 22, 2021 •

edited by amotl

Loading

amotl commented Mar 1, 2021 •

edited

Loading

amotl commented Mar 15, 2021 •

edited

Loading

amotl Mar 15, 2021 •

edited

Loading

amotl Jun 17, 2022 •

edited

Loading

amotl commented Mar 31, 2021 •

edited

Loading

amotl commented Sep 29, 2022 •

edited

Loading

amotl commented May 8, 2024 •

edited

Loading