fix(redshift): convert_dttm method for redshift dataset and tests #26283

gaurav7261 · 2023-12-15T17:00:19Z

add convert_dttm method for redshift

SUMMARY

There was convert_dttm implementation missing for redshift
due to this date filter on redshit dataset not working
giving sqlalchemy error raise exc.CompileError( sqlalchemy.exc.CompileError: Cannot compile Column object until its 'name' is assigned.

query that was getting submitted: 10:20:33,526:INFO:superset.connectors.sqla.models:SELECT day AS day FROM (SELECT date_trunc('day', created_on) as day FROM public.redshift_order) AS virtual_table WHERE day IN (<name unknown>)

BEFORE SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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

add convert_dttm method for redshift

john-bodley · 2023-12-15T18:43:09Z

superset/db_engine_specs/redshift.py

@@ -147,6 +151,19 @@ def _mutate_label(label: str) -> str:
        """
        return label.lower()

+    @classmethod


This seems to be identical with the PostgresEngineSpec.convert_dttm() method which it inherits and thus this isn't needed.

Keeping the unit tests makes sense.

but same postgres method is not getting called, when i use it?

that's why if i remove method, test is getting failed as well

i understand it inherits postgres, but somehow mixin is not working @john-bodley

@gaurav7261 you're correct - @john-bodley my bad, convert_dttm is actually defined in PostgresEngineSpec, not PostgresBaseEngineSpec like I said a moment ago. So this change LGTM.

@john-bodley @villebro can you help why precommit failed,what i need to do

Did you check the details of the CI workflow? https://github.com/apache/superset/actions/runs/7226045710/job/19691266350?pr=26283

It specifies which changes are needed. Tip: you can configure pre-commit to do this automatically for you. Check here for details: https://github.com/apache/superset/blob/master/CONTRIBUTING.md#git-hooks

thanks, got it, actually i was confused when it highlight the line on another function get_cancel_query_id

@john-bodley please approve the run

codecov · 2023-12-15T18:45:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (429e2a3) 66.99% compared to head (65dc201) 69.18%.
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26283      +/-   ##
==========================================
+ Coverage   66.99%   69.18%   +2.18%     
==========================================
  Files        1945     1945              
  Lines       75949    75950       +1     
  Branches     8460     8460              
==========================================
+ Hits        50885    52544    +1659     
+ Misses      22879    21221    -1658     
  Partials     2185     2185

Flag	Coverage Δ
hive	`53.67% <41.66%> (?)`
mysql	`78.06% <41.66%> (-0.01%)`	⬇️
postgres	`78.18% <83.33%> (-0.01%)`	⬇️
presto	`53.62% <41.66%> (?)`
python	`82.86% <100.00%> (+4.54%)`	⬆️
sqlite	`76.83% <41.66%> (-0.01%)`	⬇️
unit	`55.79% <100.00%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

john-bodley · 2023-12-15T22:43:46Z

@gaurav7261 CI failing. If you run tox -e pre-commit locally it will rectify the formatting issue.

villebro · 2023-12-16T04:28:19Z

superset/db_engine_specs/redshift.py

+    def convert_dttm(
+        cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None
+    ) -> str | None:
+        sqla_type = cls.get_sqla_column_type(target_type)
+
+        if isinstance(sqla_type, Date):
+            return f"TO_DATE('{dttm.date().isoformat()}', 'YYYY-MM-DD')"
+        if isinstance(sqla_type, DateTime):
+            dttm_formatted = dttm.isoformat(sep=" ", timespec="microseconds")
+            return f"""TO_TIMESTAMP('{dttm_formatted}', 'YYYY-MM-DD HH24:MI:SS.US')"""
+        return None


It hit me, we should consider moving this method from PostgresEngineSpec to PostgresBaseEngineSpec, as I think it's probably a good default for Postgres-like databases. This way the Redshift spec would get it "for free".

yes sure, doing that

done @john-bodley @villebro , redshift test is also running, i have ran tox -e pre-commit as well @john-bodley , please approve CI

@john-bodley i think ci is not started yet, can you help

…neSpec

gaurav7261 · 2023-12-20T19:44:50Z

@john-bodley can we merge this pr ?

villebro · 2023-12-20T23:11:53Z

@michael-s-molina can we add this to the 3.x releases?

michael-s-molina · 2023-12-21T11:44:44Z

@michael-s-molina can we add this to the 3.x releases?

@villebro All fixes that are applicable to a release are automatically cherry-picked once merged to master and released in the next patch. You don't need to request their addition 😉

…6283) Co-authored-by: GauravM <[email protected]> (cherry picked from commit 60abf7e)

…ache#26283) Co-authored-by: GauravM <[email protected]> (cherry picked from commit 60abf7e)

…ache#26283) Co-authored-by: GauravM <[email protected]>

convert_dttm method for redshift dataset and test

23590b5

add convert_dttm method for redshift

pull-request-size bot added the size/M label Dec 15, 2023

gaurav7261 mentioned this pull request Dec 15, 2023

fix(dashboard): load redshift date filter correctly #26271

Closed

9 tasks

gaurav7261 closed this Dec 15, 2023

gaurav7261 reopened this Dec 15, 2023

john-bodley reviewed Dec 15, 2023

View reviewed changes

gaurav7261 changed the title ~~convert_dttm method for redshift dataset and test~~ fix(redshift): convert_dttm method for redshift dataset and tests Dec 15, 2023

[FIX] liniting

8c7ff26

villebro approved these changes Dec 15, 2023

View reviewed changes

[CHORE] fix imports

e1de101

villebro reviewed Dec 16, 2023

View reviewed changes

GauravM added 2 commits December 20, 2023 21:09

[CHORE] move convert_dttm from PostgresEngineSpec to PostgresBaseEngi…

a52de05

…neSpec

[CHORE] remove unused imports

65dc201

john-bodley approved these changes Dec 20, 2023

View reviewed changes

john-bodley merged commit 60abf7e into apache:master Dec 20, 2023
33 checks passed

michael-s-molina added v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Dec 21, 2023

michael-s-molina pushed a commit that referenced this pull request Dec 27, 2023

fix(redshift): convert_dttm method for redshift dataset and tests (#2…

05bf190

…6283) Co-authored-by: GauravM <[email protected]> (cherry picked from commit 60abf7e)

michael-s-molina pushed a commit that referenced this pull request Dec 27, 2023

fix(redshift): convert_dttm method for redshift dataset and tests (#2…

c19447e

…6283) Co-authored-by: GauravM <[email protected]> (cherry picked from commit 60abf7e)

josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024

fix(redshift): convert_dttm method for redshift dataset and tests (ap…

fae8883

…ache#26283) Co-authored-by: GauravM <[email protected]> (cherry picked from commit 60abf7e)

mistercrunch added 🍒 3.0.3 🍒 3.0.4 🍒 3.1.0 labels Mar 8, 2024

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

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

fix(redshift): convert_dttm method for redshift dataset and tests (ap…

c5736e2

…ache#26283) Co-authored-by: GauravM <[email protected]>

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix(redshift): convert_dttm method for redshift dataset and tests (ap…

f657803

…ache#26283) Co-authored-by: GauravM <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(redshift): convert_dttm method for redshift dataset and tests #26283

fix(redshift): convert_dttm method for redshift dataset and tests #26283

gaurav7261 commented Dec 15, 2023

john-bodley Dec 15, 2023

gaurav7261 Dec 15, 2023

gaurav7261 Dec 15, 2023

gaurav7261 Dec 15, 2023

villebro Dec 15, 2023

gaurav7261 Dec 15, 2023

villebro Dec 15, 2023

gaurav7261 Dec 15, 2023

gaurav7261 Dec 15, 2023

codecov bot commented Dec 15, 2023 •

edited

Loading

john-bodley commented Dec 15, 2023

villebro Dec 16, 2023

john-bodley Dec 16, 2023

gaurav7261 Dec 16, 2023

gaurav7261 Dec 20, 2023

gaurav7261 Dec 20, 2023

gaurav7261 commented Dec 20, 2023

villebro commented Dec 20, 2023

michael-s-molina commented Dec 21, 2023

fix(redshift): convert_dttm method for redshift dataset and tests #26283

fix(redshift): convert_dttm method for redshift dataset and tests #26283

Conversation

gaurav7261 commented Dec 15, 2023

SUMMARY

BEFORE SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2023 • edited Loading

Codecov Report

john-bodley commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurav7261 commented Dec 20, 2023

villebro commented Dec 20, 2023

michael-s-molina commented Dec 21, 2023

codecov bot commented Dec 15, 2023 •

edited

Loading