Sqlalchemy 2.0 #6671

AndrewChubatiuk · 2023-12-21T19:25:36Z

What type of PR is this?

Upgraded SQLAlchemy 1.3 -> 2.0

How is this tested?

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

masayuki038

@AndrewChubatiuk Thanks for the PR.
However, there are many changes and it appears that the SQL Alchemy version update includes unrelated refactorings...

We would like to accurately manage what modifications have been made with SQL Alchemy version upgrades, so could you please separate the PR? I apologize for asking you to do something troublesome, but it would be a great help if you could do that. thank you.

masayuki038 · 2023-12-22T09:47:24Z

@guidopetri @konnectr If it is okay to merge this from your perspective, please let me know. If so, I will consider this again.

AndrewChubatiuk · 2023-12-22T10:20:37Z

@masayuki038 besides:

removing -p option in ci
moving Alerts states to a separate Enum

other changes were required:

paginator changes were needed cause paginator function from flask-sqlachemy has changed
removing relative imports was related to issues in sqlalchemy
sort_query function was completely removed from sqlalchemy-utils, so it had to be replaced
all json changes were related to a bunch of issues in tests with a filtering in where clause

sqlalchemy dependencies introduce lots of changes, which do not allow to prepare tiny diffs. god bless flask and all its dependencies was not required to be upgraded together with sqlalchemy deps

AndrewChubatiuk · 2023-12-22T10:21:54Z

@masayuki038 could you please trigger ci from the latest commit, I've fixed lint there

masayuki038 · 2023-12-22T12:24:47Z

It looks ci is running now...
https://github.com/getredash/redash/actions/runs/7298330872

AndrewChubatiuk · 2023-12-22T15:09:41Z

It looks ci is running now... https://github.com/getredash/redash/actions/runs/7298330872

build for a latest commit was not triggered

For some reason, CI stopped working, so I will cancel the review.

masayuki038 · 2023-12-23T02:07:05Z

@AndrewChubatiuk I manually re-run Github Action, but I may not have been able to fetch the latest code. I tried deleting my review, but the status did not change.

Could you try pushing something? Or could you please close this and create another PR?

AndrewChubatiuk · 2023-12-23T08:03:07Z

@masayuki038 please approve this pipeline
https://github.com/getredash/redash/actions/runs/7307066364

AndrewChubatiuk · 2023-12-23T09:55:59Z

@masayuki038 some changes were lost during rebase. pushed once again

codecov · 2023-12-23T11:47:52Z

Codecov Report

Attention: Patch coverage is 87.09677% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 63.90%. Comparing base (b7f22b1) to head (05bbe48).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6671      +/-   ##
==========================================
+ Coverage   63.80%   63.90%   +0.09%     
==========================================
  Files         161      162       +1     
  Lines       13087    13175      +88     
  Branches     1812     1824      +12     
==========================================
+ Hits         8350     8419      +69     
- Misses       4434     4446      +12     
- Partials      303      310       +7

Files	Coverage Δ
redash/alerts.py	`100.00% <100.00%> (ø)`
redash/app.py	`100.00% <100.00%> (ø)`
redash/authentication/__init__.py	`81.44% <100.00%> (+0.09%)`	⬆️
redash/cli/organization.py	`91.66% <100.00%> (+0.23%)`	⬆️
redash/destinations/asana.py	`81.48% <100.00%> (ø)`
redash/handlers/alerts.py	`86.74% <100.00%> (+0.16%)`	⬆️
redash/handlers/data_sources.py	`72.05% <100.00%> (ø)`
redash/handlers/destinations.py	`77.92% <100.00%> (ø)`
redash/handlers/embed.py	`87.50% <100.00%> (ø)`
redash/handlers/organization.py	`81.81% <100.00%> (+1.81%)`	⬆️
... and 41 more

konnectr · 2023-12-24T04:41:48Z

I understand correctly that we are still abandoning simplejson ?

konnectr · 2023-12-24T04:47:43Z

I will try to test the operation in manual mode in the near future. It is unlikely that our auto-tests cover all possible problems

masayuki038 · 2023-12-24T07:39:13Z

@konnectr Thanks! I will start to review this next week as well.

masayuki038

@AndrewChubatiuk
Sorry for the delay. I have finished reviewing this for now. Please check the comments.

Regarding the following points, I am not pointing out every line. If you will fix it, fix it all.

Please do not include updates that are not directly related to upgrading to SQLAlchemy 1.4 in this PR
- However, I agree with your updates almost. Please separate to another PR
In some places, count() has been changed to len(...). Please use count()

thank you.

.github/workflows/ci.yml

redash/handlers/base.py

migrations/versions/969126bd800f_.py

migrations/versions/98af61feea92_add_encrypted_options_to_data_sources.py

redash/cli/organization.py

redash/monitor.py

redash/query_runner/databend.py

redash/tasks/queries/execution.py

tests/models/test_dashboards.py

.github/workflows/ci.yml

client/cypress/cypress.js

client/cypress/integration/dashboard/sharing_spec.js

AndrewChubatiuk · 2024-04-29T15:22:18Z

@masayuki038 already checked them. answered or made code changes

redash/models/users.py

redash/tasks/queries/maintenance.py

tests/handlers/test_widgets.py

redash/tasks/queries/execution.py

masayuki038 · 2024-05-01T12:27:10Z

redash/tasks/queries/execution.py

@@ -224,12 +222,10 @@ def run(self):
                utcnow(),
            )

-            updated_query_ids = models.Query.update_latest_result(query_result)


I missed this update... Could you let me know why you removed this line?

QueryResult and Query have relationship, there's no need to run multiple queries to store both separately (especially, when Query object already exists), doing everything in store_result function

It looks like that Query is updated in update_latest_result.

redash/redash/models/__init__.py

Line 741 in b7f22b1

db.session.add(q)

Can we skip this update?

update_latest_result method was removed

Query was updated in the method(as below).

redash/redash/models/__init__.py

Lines 737 to 741 in bd17662

for q in queries:

q.latest_query_data = query_result

# don't auto-update the updated_at timestamp

q.skip_updated_at = True

db.session.add(q)

Is there an alternative to this? Or have you decided that this process is not necessary? If you decide that it is unnecessary, please let us know the reason.

It seems to me that without this update, Redash's behavior will be affected. Isn't Query.lastest_query_data and some attributes no longer updated?

i do not see contradiction, from what you've posted.

Referenced by: TABLE "queries" CONSTRAINT "queries_latest_query_data_id_fkey" FOREIGN KEY (latest_query_data_id) REFERENCES query_results(id)

so 1 QueryResult to many Queries

I don't think that having Foreign Key constraints has anything to do with multiplicity.

I thought 1 Query to many QueryResults (like this).

Is it not correct?

Query has latest_query_data_id attribute, which references private key in QueryResult table, which means, that multiple Query object can set the same latest_query_data_id, which means that 1 QueryResult will be referred by multiple Queries - 1 QueryResults many queries

hey @masayuki038
sorry for time you've spend in a review process, but closing this PR as not going to work on redash any further

which means, that multiple Query object can set the same latest_query_data_id,

This is the case if different Queries have the same query_hash. However, it is caused by hash value collisions, right? I don't think we should discuss it in this context.

1 Query - many QueryResults

Yes. Therefore, I think we should name query instead of queries in QueryResult. That is my point.

redash/handlers/queries.py

redash/handlers/users.py

tests/test_authentication.py

tests/metrics/test_request.py

masayuki038 · 2024-05-02T12:20:28Z

@AndrewChubatiuk Could you stop to update the things other than "Sqlalchemy 2.0"?

AndrewChubatiuk · 2024-05-02T12:28:30Z

@AndrewChubatiuk Could you stop to update the things other than "Sqlalchemy 2.0"?

@masayuki038 i update only things, which are impacted by pr comments changes

masayuki038 · 2024-05-02T12:32:42Z

@AndrewChubatiuk OK. But, I don't think we need to update "assert_called_once_with("requests.redash_ping.get", ANY)" to "assert_called" like the above thread by upgrading to SQLAlchemy 2.0... Why did you update it yesterday?

AndrewChubatiuk · 2024-05-02T12:49:17Z

@AndrewChubatiuk OK. But, I don't think we need to update "assert_called_once_with("requests.redash_ping.get", ANY)" to "assert_called" like the above thread by upgrading to SQLAlchemy 2.0... Why did you update it yesterday?

replied in a thread

AndrewChubatiuk mentioned this pull request Dec 21, 2023

upgraded sqlalchemy deps to versions with wheels to speed up arm64 builds #6637

Closed

1 task

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from fcb824b to 3b9234b Compare December 22, 2023 09:27

masayuki038 previously requested changes Dec 22, 2023

View reviewed changes

masayuki038 self-requested a review December 22, 2023 17:19

masayuki038 removed their request for review December 22, 2023 17:31

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from 1c04150 to d3391c3 Compare December 23, 2023 08:01

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from d3391c3 to bfd23f6 Compare December 23, 2023 09:54

masayuki038 requested changes Dec 28, 2023

View reviewed changes

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 2 times, most recently from e6bd1a6 to d46a475 Compare January 3, 2024 08:33

masayuki038 reviewed Jan 3, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

client/cypress/cypress.js Outdated Show resolved Hide resolved

masayuki038 reviewed Jan 3, 2024

View reviewed changes

client/cypress/integration/dashboard/sharing_spec.js Outdated Show resolved Hide resolved

This was referenced Jan 4, 2024

removed Configuration class #6682

Merged

removed simplejson #6685

Merged

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 2 times, most recently from 6d6cdcf to dfa3da8 Compare January 9, 2024 19:55

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from dfa3da8 to 8d118f2 Compare January 11, 2024 23:34

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from 66056ec to f30a674 Compare April 29, 2024 15:21

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 4 times, most recently from e897b4d to 8398d6e Compare April 29, 2024 19:32

masayuki038 reviewed Apr 30, 2024

View reviewed changes

redash/models/users.py Outdated Show resolved Hide resolved

masayuki038 reviewed Apr 30, 2024

View reviewed changes

redash/tasks/queries/maintenance.py Outdated Show resolved Hide resolved

masayuki038 reviewed Apr 30, 2024

View reviewed changes

tests/handlers/test_widgets.py Show resolved Hide resolved

masayuki038 reviewed May 1, 2024

View reviewed changes

redash/tasks/queries/execution.py Outdated Show resolved Hide resolved

masayuki038 reviewed May 1, 2024

View reviewed changes

redash/handlers/queries.py Outdated Show resolved Hide resolved

masayuki038 reviewed May 1, 2024

View reviewed changes

redash/handlers/users.py Outdated Show resolved Hide resolved

masayuki038 reviewed May 1, 2024

View reviewed changes

tests/test_authentication.py Outdated Show resolved Hide resolved

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 2 times, most recently from d743625 to 963fa82 Compare May 1, 2024 20:46

sqlalchemy 2.0

41e6cb6

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 2 times, most recently from 5f3f973 to 80dab5a Compare May 2, 2024 12:13

masayuki038 requested changes May 2, 2024

View reviewed changes

tests/metrics/test_request.py Outdated Show resolved Hide resolved

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch 3 times, most recently from f547a75 to e3cb28c Compare May 3, 2024 01:12

pr comments

05bbe48

AndrewChubatiuk force-pushed the sqlalchemy-1.4 branch from e3cb28c to 05bbe48 Compare May 4, 2024 11:37

AndrewChubatiuk closed this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sqlalchemy 2.0 #6671

Sqlalchemy 2.0 #6671

AndrewChubatiuk commented Dec 21, 2023 •

edited

masayuki038 left a comment •

edited

masayuki038 commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

masayuki038 commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

masayuki038 commented Dec 23, 2023

AndrewChubatiuk commented Dec 23, 2023

AndrewChubatiuk commented Dec 23, 2023

codecov bot commented Dec 23, 2023 •

edited

konnectr commented Dec 24, 2023

konnectr commented Dec 24, 2023

masayuki038 commented Dec 24, 2023 •

edited

masayuki038 left a comment •

edited

AndrewChubatiuk commented Apr 29, 2024 •

edited

masayuki038 May 1, 2024

AndrewChubatiuk May 1, 2024

masayuki038 May 2, 2024

AndrewChubatiuk May 2, 2024

masayuki038 May 3, 2024 •

edited

AndrewChubatiuk May 7, 2024

masayuki038 May 8, 2024

AndrewChubatiuk May 8, 2024 •

edited

AndrewChubatiuk May 9, 2024

masayuki038 May 9, 2024

masayuki038 commented May 2, 2024

AndrewChubatiuk commented May 2, 2024

masayuki038 commented May 2, 2024 •

edited

AndrewChubatiuk commented May 2, 2024

	for q in queries:
	q.latest_query_data = query_result
	# don't auto-update the updated_at timestamp
	q.skip_updated_at = True
	db.session.add(q)

Sqlalchemy 2.0 #6671

Sqlalchemy 2.0 #6671

Conversation

AndrewChubatiuk commented Dec 21, 2023 • edited

What type of PR is this?

How is this tested?

masayuki038 left a comment • edited

Choose a reason for hiding this comment

masayuki038 commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

masayuki038 commented Dec 22, 2023

AndrewChubatiuk commented Dec 22, 2023

masayuki038 commented Dec 23, 2023

AndrewChubatiuk commented Dec 23, 2023

AndrewChubatiuk commented Dec 23, 2023

codecov bot commented Dec 23, 2023 • edited

Codecov Report

konnectr commented Dec 24, 2023

konnectr commented Dec 24, 2023

masayuki038 commented Dec 24, 2023 • edited

masayuki038 left a comment • edited

Choose a reason for hiding this comment

AndrewChubatiuk commented Apr 29, 2024 • edited

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

masayuki038 May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewChubatiuk May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayuki038 commented May 2, 2024

AndrewChubatiuk commented May 2, 2024

masayuki038 commented May 2, 2024 • edited

AndrewChubatiuk commented May 2, 2024

AndrewChubatiuk commented Dec 21, 2023 •

edited

masayuki038 left a comment •

edited

codecov bot commented Dec 23, 2023 •

edited

masayuki038 commented Dec 24, 2023 •

edited

masayuki038 left a comment •

edited

AndrewChubatiuk commented Apr 29, 2024 •

edited

masayuki038 May 3, 2024 •

edited

AndrewChubatiuk May 8, 2024 •

edited

masayuki038 commented May 2, 2024 •

edited