fix: handle TIME column serialization #16869

frafra · 2021-09-28T14:37:22Z

SUMMARY

See the long comment at: #15905 (comment)

TESTING INSTRUCTIONS

Use a dataset with a TIME column (not DATETIME) and represent it using a Table view, by including the TIME column in COLUMNS.

ADDITIONAL INFORMATION

Has associated issue: Fixes Global async query - Unserializable object {} of type <class 'datetime.time'> #15905
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

frafra · 2021-09-28T14:47:42Z

I got a too-many-return-statements from pylint. Should I rework the whole cascade of if statements?

codecov · 2021-09-28T15:31:17Z

Codecov Report

Merging #16869 (8472d0f) into master (333b137) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #16869      +/-   ##
==========================================
- Coverage   76.95%   76.81%   -0.14%     
==========================================
  Files        1038     1038              
  Lines       56021    56019       -2     
  Branches     7735     7735              
==========================================
- Hits        43110    43032      -78     
- Misses      12653    12729      +76     
  Partials      258      258

Flag	Coverage Δ
hive	`?`
mysql	`81.91% <100.00%> (+<0.01%)`	⬆️
postgres	`81.92% <100.00%> (+<0.01%)`	⬆️
python	`82.01% <100.00%> (-0.27%)`	⬇️
sqlite	`81.60% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/utils/core.py	`90.08% <100.00%> (-0.03%)`	⬇️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.19%)`	⬇️
superset/db_engine_specs/hive.py	`69.76% <0.00%> (-17.06%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/db_engine_specs/presto.py	`83.47% <0.00%> (-0.84%)`	⬇️
superset/db_engine_specs/base.py	`88.20% <0.00%> (-0.39%)`	⬇️
superset/connectors/sqla/models.py	`86.35% <0.00%> (-0.24%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 333b137...8472d0f. Read the comment docs.

frafra · 2021-09-28T19:57:30Z

I tried to do a very small refactory to make pylint happy. I just grouped together the 2 (now 3) instance types requiring a serialization via str(...).

villebro · 2021-09-29T06:40:00Z

I'm surprised this is not happening with GAQ disabled - is this really the case? Long term we should probably introduce the option to format time values in the frontend like we do with TIMESTAMP/DATE/DATETIME, and to do that we would need to convert it into millisecond format like we do for datetime, introduce a new GenericDataType type called TIME that the frontend could then use to properly format the values. Is this something you would be willing to collaborate on @frafra ? In the mean time this could be a good compromise.

frafra · 2021-09-29T10:07:26Z

I do not have the time to tackle such a big effort at the moment, and this is my very first PR to Superset (excluding documentation).

jerwen · 2021-10-07T13:31:59Z

I'm surprised this is not happening with GAQ disabled - is this really the case?

In fact, the problem is also present without GAQ starting with docker image apache/superset:1.1.0.
We are currently running on apache/superset:1.0.1 without GAQ and the problem is not present.

Above is the error message you get with apache/superset:1.3.0

jerwen · 2021-10-28T09:46:37Z

@villebro Do you have an estimated horizon to merge this PR ?
We're stuck in version 1.0.1 because of the regression this PR fixes and we would like to upgrade to the latest version.

villebro · 2021-11-02T08:39:46Z

@jerwen sorry for letting this slip between the cracks 🙁 I'll look at this now and try to get it into the next release.

villebro

LGTM - tested, reproduced the error on master and retested on the PR and saw the error resolved.

villebro · 2021-11-02T09:23:39Z

@jerwen you need to rebase this PR, as we've bumped the Python version on CI from 3.7 to 3.8, hence it's expecting the status to be reported for Python 3.8 tests (which weren't run here due to CI being on 3.7 at the time of the PR being opened)

jerwen · 2021-11-10T08:42:03Z

@frafra could you rebase this PR on master please ?

jerwen · 2021-11-10T15:55:17Z

@villebro the PR should now be ready for merge 🎊
Thank you @frafra for your availability

villebro · 2021-11-11T08:35:15Z

Thanks all for helping out here!

* Add support for datetime.time in json_int_dttm_ser * Test base_json_conv support for datetime.time * Group types by conversion function for JSON dump

Add support for datetime.time in json_int_dttm_ser

c6a681a

pull-request-size bot added the size/XS label Sep 28, 2021

Test base_json_conv support for datetime.time

968be66

Group types by conversion function for JSON dump

70d0527

frafra changed the title ~~fix(core): handle TIME column~~ fix: handle TIME column Sep 28, 2021

frafra changed the title ~~fix: handle TIME column~~ fix: handle TIME column serialization Sep 28, 2021

villebro approved these changes Nov 2, 2021

View reviewed changes

Merge branch 'apache:master' into handle-time-column

8472d0f

villebro merged commit 0d77f36 into apache:master Nov 11, 2021

frafra deleted the handle-time-column branch November 11, 2021 08:51

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022

fix: handle TIME column serialization (#16869)

2171695

* Add support for datetime.time in json_int_dttm_ser * Test base_json_conv support for datetime.time * Group types by conversion function for JSON dump

eschutho added the v1.4.1 label Jan 15, 2022

eschutho pushed a commit that referenced this pull request Jan 27, 2022

fix: handle TIME column serialization (#16869)

f82ec27

* Add support for datetime.time in json_int_dttm_ser * Test base_json_conv support for datetime.time * Group types by conversion function for JSON dump

mistercrunch added 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle TIME column serialization #16869

fix: handle TIME column serialization #16869

frafra commented Sep 28, 2021 •

edited

Loading

frafra commented Sep 28, 2021

codecov bot commented Sep 28, 2021 •

edited

Loading

frafra commented Sep 28, 2021 •

edited

Loading

villebro commented Sep 29, 2021

frafra commented Sep 29, 2021

jerwen commented Oct 7, 2021

jerwen commented Oct 28, 2021

villebro commented Nov 2, 2021

villebro left a comment

villebro commented Nov 2, 2021

jerwen commented Nov 10, 2021

jerwen commented Nov 10, 2021

villebro commented Nov 11, 2021

fix: handle TIME column serialization #16869

fix: handle TIME column serialization #16869

Conversation

frafra commented Sep 28, 2021 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

frafra commented Sep 28, 2021

codecov bot commented Sep 28, 2021 • edited Loading

Codecov Report

frafra commented Sep 28, 2021 • edited Loading

villebro commented Sep 29, 2021

frafra commented Sep 29, 2021

jerwen commented Oct 7, 2021

jerwen commented Oct 28, 2021

villebro commented Nov 2, 2021

villebro left a comment

Choose a reason for hiding this comment

villebro commented Nov 2, 2021

jerwen commented Nov 10, 2021

jerwen commented Nov 10, 2021

villebro commented Nov 11, 2021

frafra commented Sep 28, 2021 •

edited

Loading

codecov bot commented Sep 28, 2021 •

edited

Loading

frafra commented Sep 28, 2021 •

edited

Loading