fix(trino): Fix Trino timestamp conversion #21737

mdesmet · 2022-10-07T14:34:45Z

SUMMARY

Remove timestamp precision for custom python dttm conversion.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: Time range filter not working with Trino. #16223, Superset does not cast trino timestamp correctly #21567
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

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!

hovaesco · 2022-10-10T09:18:23Z

@john-bodley could you please take a look and approve CI?

hashhar · 2022-10-12T13:18:12Z

tests/integration_tests/db_engine_specs/trino_tests.py

+        self.assertEqual(
+            TrinoEngineSpec.convert_dttm("TIMESTAMP WITH TIME ZONE", dttm),
+            "TIMESTAMP '2019-01-02 03:04:05.678900'",
+        )


did you mean to use TIMESTAMP(3) WITH TIME ZONE here instead?

Indeed, my mistake!

georgewfisher · 2022-10-12T20:08:20Z

I have tested this change and it addresses the timestamp filtering issue.

mayurnewase · 2022-10-14T00:59:36Z

thanks for the fix, approving CI.

codecov · 2022-10-14T01:07:48Z

Codecov Report

Merging #21737 (08f2c9b) into master (60a617e) will increase coverage by 1.29%.
The diff coverage is n/a.

❗ Current head 08f2c9b differs from pull request most recent head cef7a1e. Consider uploading reports for the commit cef7a1e to get more accurate results

@@            Coverage Diff             @@
##           master   #21737      +/-   ##
==========================================
+ Coverage   65.56%   66.86%   +1.29%     
==========================================
  Files        1847     1847              
  Lines       70558    70558              
  Branches     7735     7735              
==========================================
+ Hits        46264    47181     +917     
+ Misses      22294    21377     -917     
  Partials     2000     2000

Flag	Coverage Δ
hive	`52.53% <ø> (ø)`
javascript	`53.83% <ø> (ø)`
mysql	`77.95% <ø> (ø)`
postgres	`78.02% <ø> (ø)`
presto	`52.42% <ø> (ø)`
python	`81.23% <ø> (+2.73%)`	⬆️
sqlite	`76.48% <ø> (ø)`
unit	`50.91% <ø> (?)`

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

Impacted Files	Coverage Δ
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	`56.60% <ø> (ø)`
superset/databases/api.py	`93.53% <0.00%> (+0.30%)`	⬆️
superset/sql_lab.py	`82.12% <0.00%> (+0.38%)`	⬆️
superset/views/base_api.py	`98.44% <0.00%> (+0.38%)`	⬆️
superset/db_engine_specs/presto.py	`88.22% <0.00%> (+0.41%)`	⬆️
superset/db_engine_specs/__init__.py	`85.71% <0.00%> (+1.19%)`	⬆️
superset/datasets/schemas.py	`97.40% <0.00%> (+1.29%)`	⬆️
superset/commands/exceptions.py	`93.84% <0.00%> (+1.53%)`	⬆️
superset/initialization/__init__.py	`90.96% <0.00%> (+2.00%)`	⬆️
superset/utils/log.py	`93.91% <0.00%> (+2.02%)`	⬆️
... and 54 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

georgewfisher · 2022-11-29T22:23:12Z

@mayurnewase who needs to review this PR in order for it to be committed?

john-bodley · 2022-12-07T23:04:44Z

tests/integration_tests/db_engine_specs/trino_tests.py

+    def test_convert_dttm(self):
+        dttm = self.get_dttm()
+
+        self.assertEqual(


Using assert rather than self.assertEqual is preferred in our pytest/unittest hybrid world.

nytai · 2022-12-07T23:06:37Z

superset/db_engine_specs/trino.py

+        tt = target_type.upper()
+        if tt == utils.TemporalType.DATE:
+            return f"DATE '{dttm.date().isoformat()}'"
+        if re.sub(r"\(\d\)", "", tt) in (


is there a reason this needs to be handled differently than the method already defined in PrestoBaseEngineSpec?

superset/superset/db_engine_specs/presto.py

Line 206 in 08f2c9b

if tt in (

Trino exposes the precision in the temporal types, this is not covered in the Presto code (this is what's the bug was about)

john-bodley · 2022-12-07T23:10:23Z

superset/db_engine_specs/trino.py

@@ -47,6 +49,29 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
    engine = "trino"
    engine_name = "Trino"

+    @classmethod
+    def convert_dttm(


Given that Presto and Trino have diverged in therms of their TIMESTAMP functions I wonder if the PrestoBaseEngineSpec.convert_dttm method should be moved to the PrestoEngineSpec class.

Makes sense. I'll take care of that.

EugeneTorap · 2022-12-08T08:49:10Z

@mdesmet Thank you so much for this PR!
Can update the description and use Fix before the ticket link in order to auto close these two tickets after PR merging

(cherry picked from commit 90d79c7)

pull-request-size bot added the size/M label Oct 7, 2022

github-actions bot reviewed Oct 7, 2022

View reviewed changes

hovaesco approved these changes Oct 10, 2022

View reviewed changes

hashhar reviewed Oct 12, 2022

View reviewed changes

mdesmet force-pushed the bug/trino-convert_dttm branch from 2a07157 to 54236d3 Compare October 13, 2022 13:15

mdesmet force-pushed the bug/trino-convert_dttm branch from 54236d3 to 91e04c5 Compare November 14, 2022 14:30

Fix Trino timestamp conversion

cef7a1e

mdesmet force-pushed the bug/trino-convert_dttm branch from 91e04c5 to cef7a1e Compare December 7, 2022 18:58

rusackas requested review from bkyryliuk, john-bodley and nytai December 7, 2022 23:00

nytai approved these changes Dec 7, 2022

View reviewed changes

john-bodley reviewed Dec 7, 2022

View reviewed changes

nytai reviewed Dec 7, 2022

View reviewed changes

john-bodley reviewed Dec 7, 2022

View reviewed changes

rusackas merged commit 90d79c7 into apache:master Dec 14, 2022

mdesmet deleted the bug/trino-convert_dttm branch December 14, 2022 18:33

This was referenced Jan 3, 2023

chore: Migrate /superset/search_queries to API v1 #22579

Merged

chore: Migrate /superset/queries/<last_updated_ms> to API v1 #22611

Merged

john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jan 18, 2023

fix(trino): Fix Trino timestamp conversion (apache#21737)

3d9ba46

(cherry picked from commit 90d79c7)

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

mistercrunch removed the 🚢 2.1.3 label Mar 13, 2024

okayhooni mentioned this pull request May 24, 2024

BUG) Superset get wrong value of TIMESTAMP WITH TIME ZONE type field on Trino client #28685

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(trino): Fix Trino timestamp conversion #21737

fix(trino): Fix Trino timestamp conversion #21737

mdesmet commented Oct 7, 2022 •

edited

Loading

github-actions bot left a comment

hovaesco commented Oct 10, 2022

hashhar Oct 12, 2022

mdesmet Oct 12, 2022

georgewfisher commented Oct 12, 2022

mayurnewase commented Oct 14, 2022

codecov bot commented Oct 14, 2022 •

edited

Loading

georgewfisher commented Nov 29, 2022

john-bodley Dec 7, 2022

nytai Dec 7, 2022

mdesmet Dec 7, 2022

john-bodley Dec 7, 2022

mdesmet Dec 7, 2022

EugeneTorap commented Dec 8, 2022 •

edited

Loading

fix(trino): Fix Trino timestamp conversion #21737

fix(trino): Fix Trino timestamp conversion #21737

Conversation

mdesmet commented Oct 7, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

github-actions bot left a comment

Choose a reason for hiding this comment

hovaesco commented Oct 10, 2022

hashhar Oct 12, 2022

Choose a reason for hiding this comment

mdesmet Oct 12, 2022

Choose a reason for hiding this comment

georgewfisher commented Oct 12, 2022

mayurnewase commented Oct 14, 2022

codecov bot commented Oct 14, 2022 • edited Loading

Codecov Report

georgewfisher commented Nov 29, 2022

john-bodley Dec 7, 2022

Choose a reason for hiding this comment

nytai Dec 7, 2022

Choose a reason for hiding this comment

mdesmet Dec 7, 2022

Choose a reason for hiding this comment

john-bodley Dec 7, 2022

Choose a reason for hiding this comment

mdesmet Dec 7, 2022

Choose a reason for hiding this comment

EugeneTorap commented Dec 8, 2022 • edited Loading

mdesmet commented Oct 7, 2022 •

edited

Loading

codecov bot commented Oct 14, 2022 •

edited

Loading

EugeneTorap commented Dec 8, 2022 •

edited

Loading