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

removed PseudoJSON #6687

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Jan 3, 2024

What type of PR is this?

Removed unneeded PseudoJSON and DBPersistence classes, migrated all json contained column's type to JSONB

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

How is this tested?

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

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 solves a long-awaited TODO!

Can we also convert query_results.data to JSONB?

redash/models/__init__.py Outdated Show resolved Hide resolved
@AndrewChubatiuk AndrewChubatiuk force-pushed the removed-pseudojson branch 2 times, most recently from b643c2f to f524ab3 Compare January 8, 2024 10:47
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 142 lines in your changes are missing coverage. Please review.

Comparison is base (4d51039) 63.30% compared to head (e040508) 63.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6687      +/-   ##
==========================================
+ Coverage   63.30%   63.38%   +0.07%     
==========================================
  Files         162      162              
  Lines       13322    13165     -157     
  Branches     1820     1817       -3     
==========================================
- Hits         8434     8344      -90     
+ Misses       4599     4532      -67     
  Partials      289      289              
Files Coverage Δ
redash/handlers/base.py 93.02% <100.00%> (ø)
redash/handlers/dashboards.py 77.39% <ø> (ø)
redash/handlers/visualizations.py 100.00% <ø> (+5.12%) ⬆️
redash/models/__init__.py 91.95% <100.00%> (+0.10%) ⬆️
redash/models/base.py 86.66% <100.00%> (ø)
redash/models/changes.py 90.19% <100.00%> (ø)
redash/models/organizations.py 83.87% <100.00%> (+0.26%) ⬆️
redash/models/types.py 88.46% <ø> (-0.43%) ⬇️
redash/models/users.py 94.80% <100.00%> (ø)
redash/query_runner/influx_db.py 67.16% <100.00%> (-0.49%) ⬇️
... and 65 more

pyproject.toml Outdated Show resolved Hide resolved
eradman
eradman previously approved these changes Jan 9, 2024
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 change appears to be in good shape

@eradman
Copy link
Collaborator

eradman commented Jan 11, 2024

@justinclift what do you think about waving the code coverage check? It would be very difficult to make this check pass with a change like this

@justinclift
Copy link
Member

@eradman Yeah, the code coverage stuff is definitely a case of "don't worry about it in situations where it doesn't apply". 😄

@justinclift justinclift merged commit ec1c4d0 into getredash:master Jan 11, 2024
14 of 15 checks passed
@justinclift
Copy link
Member

It looks like using jsonb is having some unintended consequences for people with large data sets: #6706 (comment)

We might need to revert this change.

@eradman
Copy link
Collaborator

eradman commented Jan 17, 2024

I beleive we can change query_results.data back to a text field and the rest of this change can stay as is.

@AndrewChubatiuk can you explore that solution?

@AndrewChubatiuk
Copy link
Contributor Author

@eradman @justinclift #6713

@AndrewChubatiuk AndrewChubatiuk deleted the removed-pseudojson branch January 18, 2024 15:19
wlach added a commit to wlach/redash that referenced this pull request Mar 28, 2024
Since getredash#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 decimal types that are occasionally
returned by (at least) the PostgreSQL query runner:

https://www.psycopg.org/docs/faq.html#problems-with-type-conversions
@wlach wlach mentioned this pull request Mar 28, 2024
10 tasks
justinclift pushed a commit that referenced this pull request Mar 29, 2024
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 decimal types that are occasionally
returned by (at least) the PostgreSQL query runner:

https://www.psycopg.org/docs/faq.html#problems-with-type-conversions
@wlach wlach mentioned this pull request Apr 2, 2024
10 tasks
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