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

Deduplicate queries retrieve builder view #163

Merged
merged 5 commits into from
Jan 16, 2017

Conversation

brunobord
Copy link
Contributor

@brunobord brunobord commented Jan 16, 2017

refs #157

@brunobord brunobord changed the title Depublicate queries retrieve builder view Deduplicate queries retrieve builder view Jan 16, 2017
Copy link
Contributor

@moumoutte moumoutte left a comment

Choose a reason for hiding this comment

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

LGTM

UpdateFormTestCase.test_queryset_on_get:
- db: 'SELECT ... FROM "formidable_formidable" WHERE "formidable_formidable"."id" = #'
- db: SELECT ... FROM "formidable_field" WHERE "formidable_field"."form_id" IN (#) ORDER BY "formidable_field"."order" ASC
- db: 'SELECT ... FROM "formidable_field" WHERE "formidable_field"."form_id" = # ORDER BY "formidable_field"."order" ASC'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum seems to have an extra queries here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmmmm... I don't know, I didn't try to change anything on your code, only the .yml files ; it's possible that your previous perf PRs had a side-effect on the queries generated in the next perf PRs.
Maybe you'd want to add commits to this branch and eventually tackle one last performance issue? or we may merge "as is", and we open an issue to resolve this one? your choice, this PR is the last one to integrate into django-formidable, it won't have any bad side-effect, IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I miss this one , do you mind if I add a commit ?

@brunobord brunobord force-pushed the 157_deduplicate_queries_retrieve_builder_view branch from f6f5a3e to dbb8e64 Compare January 16, 2017 16:37
@brunobord brunobord merged commit 7dfdbec into master Jan 16, 2017
@brunobord brunobord deleted the 157_deduplicate_queries_retrieve_builder_view branch January 16, 2017 16:44
brunobord added a commit that referenced this pull request Jan 17, 2017
Changelog

* Added a make target to install the demo site (#152).
* Added django-perf-rec module for tests and improved SQL queries in `ContextFormDetailView` (#54, #154, #160).
* Added test to count queries on dynamic form queryset + improve performances (#155, #156, #162).
* Added test to count queries on retrieve builder view + improve performances by removing duplicate queries (#157, #158, #163).
@brunobord brunobord mentioned this pull request Jan 17, 2017
@wo0dyn wo0dyn removed their request for review January 23, 2017 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants