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

Handle timedelta in query results #6846

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

wlach
Copy link
Contributor

@wlach wlach commented Apr 2, 2024

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

(this is a follow-on to #6837)

Since #6687, we don't serialize query results as JSON before returning them. This is fine, except for the
query results data source which needs to pass the data directly to sqlite3, and doesn't know how to
do that with the timedelta types that are occasionally returned by (at least) the PostgreSQL query runner:

https://www.psycopg.org/docs/faq.html#problems-with-type-conversions

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

N/A

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@wlach
Copy link
Contributor Author

wlach commented Apr 2, 2024

@justinclift @eradman Another one to look at when you have a sec

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.50%. Comparing base (38a06c7) to head (bbad55e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6846   +/-   ##
=======================================
  Coverage   63.50%   63.50%           
=======================================
  Files         163      163           
  Lines       13206    13209    +3     
  Branches     1824     1825    +1     
=======================================
+ Hits         8386     8389    +3     
  Misses       4518     4518           
  Partials      302      302           
Files Coverage Δ
redash/query_runner/query_results.py 63.23% <100.00%> (+0.82%) ⬆️

@justinclift
Copy link
Member

justinclift commented Apr 3, 2024

@eradman Are you ok to look over this one?

Looking over it, it seems super simple. 😁

Copy link
Collaborator

@eradman eradman left a comment

Choose a reason for hiding this comment

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

This looks good; effectively the same as casting to text in the query.

@eradman eradman marked this pull request as ready for review April 3, 2024 14:25
@eradman eradman enabled auto-merge (squash) April 3, 2024 14:27
@eradman eradman merged commit 702a550 into getredash:master Apr 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants