Refactor SQL and use ORM queries instead #139

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
@atodorov
Contributor

atodorov commented Jan 31, 2017

Supersedes #135 by removing all hard-coded SQL statements and replacing them with Django ORM queries. I have split changes into separate commits for easier reviewing but can squash them into one bigger commit before merging if you like.

In this particular instance the ORM queries result in the same SQL statements being sent to the DB backend so there should be no change in performance constraints. There is even a slight improvement at a couple of places where I've gotten rid of secondary hits to DB or fetching duplicate rows when they were not needed.

@tkdchen please review and let me know if you want me to squash all commits together.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Feb 6, 2017

Contributor

ping. any update here ?

Contributor

atodorov commented Feb 6, 2017

ping. any update here ?

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Feb 6, 2017

Member

Sorry, so far, it is not necessary to convert these SQL queries back to ORM. It is not time to merge these patches now.

Member

tkdchen commented Feb 6, 2017

Sorry, so far, it is not necessary to convert these SQL queries back to ORM. It is not time to merge these patches now.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Feb 6, 2017

Contributor

OK, I understand it is not priority to do this change right now, but the patch looks good and doesn't actually change the underlying queries. It doesn't bring any negative impact but on the other hand makes Nitrate more portable. I don't understand why not simply merge.

Contributor

atodorov commented Feb 6, 2017

OK, I understand it is not priority to do this change right now, but the patch looks good and doesn't actually change the underlying queries. It doesn't bring any negative impact but on the other hand makes Nitrate more portable. I don't understand why not simply merge.

@atodorov atodorov referenced this pull request Mar 2, 2017

Closed

Fix sqls #167

atodorov added some commits Jan 31, 2017

Replace STATS_CASERUNS_STATUS SQL with ORM query
the ORM query results in the same SQL statement and has the same
performance impact but makes the application more portable.
Replace GET_RUN_BUG_IDS SQL with ORM query
the result from this query was filtered into a set to get the
distinct values only. We do better by applying .distinct() directly
to the resulting query reducing the number of records fetched via
the DB!
Replace GET_CASERUNS_COMMENTS SQL with Django ORM query
also don't select comment author in this query because it is not
used anywhere!

atodorov added a commit to MrSenko/Nitrate that referenced this pull request May 24, 2017

Refactor SQL and use ORM queries
Relates to #139.

I need to modify this function to return more data and need this
change because PR #139 has been sitting open for more than 3 months.
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 27, 2017

Member

I'll merge these commits one by one as I'm replacing REQUEST and adding tests in testruns app.

Member

tkdchen commented May 27, 2017

I'll merge these commits one by one as I'm replacing REQUEST and adding tests in testruns app.

@tkdchen tkdchen referenced this pull request Jul 3, 2017

Merged

Update pr #139 #214

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Jul 4, 2017

Member

Commits except 12584d4 are merged. What 4633955 fixes are fixed in those merged commits.

Thanks.

Member

tkdchen commented Jul 4, 2017

Commits except 12584d4 are merged. What 4633955 fixes are fixed in those merged commits.

Thanks.

@tkdchen tkdchen closed this Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment