Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add format for timestamp in crdb to correctly use times values #24711

Merged
merged 6 commits into from
Aug 31, 2023

Conversation

elewis787
Copy link
Contributor

SUMMARY

This PR fixes #15825

TESTING INSTRUCTIONS

To test this, the following steps can be taken

  • Add db connection for cockroach db.
  • Create a dataset that includes a table with a timestamp or timestampz column, this will be used as the temporal value when creating charts.
  • Create a chart that leverages a temporal value
  • Inspect the generated query to see if the date format matches as expected.

This has been tested in master and version 2.1.0 with python 3.11.

In version 2.1.0 there were code changes needed to the dict type, this is due to a python version that was being used in the docker images of version 2.1.0.

ADDITIONAL INFORMATION

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@john-bodley
Copy link
Member

Thanks @elewis787 for the PR. Would you mind adding some unit tests? You can look at other DB engine specs for an example of how to do this.

@john-bodley john-bodley self-requested a review July 18, 2023 16:57
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #24711 (fa649bc) into master (0328dd2) will decrease coverage by 0.05%.
Report is 55 commits behind head on master.
The diff coverage is 74.65%.

❗ Current head fa649bc differs from pull request most recent head 8a06bf4. Consider uploading reports for the commit 8a06bf4 to get more accurate results

@@            Coverage Diff             @@
##           master   #24711      +/-   ##
==========================================
- Coverage   68.97%   68.92%   -0.05%     
==========================================
  Files        1901     1902       +1     
  Lines       74008    73949      -59     
  Branches     8183     8176       -7     
==========================================
- Hits        51047    50971      -76     
- Misses      20840    20865      +25     
+ Partials     2121     2113       -8     
Flag Coverage Δ
hive 54.13% <39.87%> (+0.01%) ⬆️
mysql ?
postgres 79.26% <66.87%> (-0.19%) ⬇️
presto 54.03% <39.87%> (+0.01%) ⬆️
python 83.30% <79.75%> (-0.15%) ⬇️
sqlite 77.83% <63.80%> (-0.20%) ⬇️
unit 54.97% <60.73%> (+0.13%) ⬆️

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

Files Changed Coverage Δ
...src/components/ColumnTypeLabel/ColumnTypeLabel.tsx 100.00% <ø> (ø)
...d/packages/superset-ui-chart-controls/src/index.ts 100.00% <ø> (ø)
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <ø> (ø)
...rset-ui-chart-controls/src/sections/chartTitle.tsx 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 25.00% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 50.00% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx 100.00% <ø> (ø)
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx 16.66% <ø> (ø)
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx 66.66% <ø> (ø)
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx 66.66% <ø> (ø)
... and 50 more

... and 12 files with indirect coverage changes

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

@elewis787
Copy link
Contributor Author

sure - and I will update the link errors.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 21, 2023
@elewis787
Copy link
Contributor Author

elewis787 commented Jul 21, 2023

@john-bodley tests have been added and fixed lint error for the db spec file.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you @elewis787!

@elewis787
Copy link
Contributor Author

Looks great, thank you @elewis787!

Thanks @eschutho - I pushed a few updates to hopefully fix the few linit errors that came up.

elewis787 and others added 2 commits July 27, 2023 14:16
remove dttm.

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
@elewis787
Copy link
Contributor Author

elewis787 commented Aug 12, 2023

@john-bodley / @eschutho Any updates on this ?

@sfirke
Copy link
Member

sfirke commented Aug 31, 2023

Not sure what's going on with CI here, I will close & reopen to try to trigger it

@sfirke sfirke closed this Aug 31, 2023
@sfirke sfirke reopened this Aug 31, 2023
@sfirke
Copy link
Member

sfirke commented Aug 31, 2023

@john-bodley and @eschutho can one of you peek at this and merge if OK? Looks like the feedback has been addressed and CI has passed now that I kicked it (it had stalled 3 weeks ago).

@john-bodley john-bodley merged commit 321d968 into apache:master Aug 31, 2023
31 checks passed
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 31, 2023
michael-s-molina pushed a commit that referenced this pull request Sep 1, 2023
…24711)

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
(cherry picked from commit 321d968)
darwinsubramaniam pushed a commit to darwinsubramaniam/superset that referenced this pull request Sep 7, 2023
…pache#24711)

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…pache#24711)

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[db_specific] CockroachDB does not support to_timestamp
6 participants