fix: handle TIME column serialization

[frafra]

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]

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

[codecov]

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]

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]

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]

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]

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]

@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]

@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]

@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]

@frafra could you rebase this PR on master please ?

[jerwen]

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

[villebro]

Thanks all for helping out here!