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

Use primary-key only count for Schedules B and E #4619

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

lbeaufort
Copy link
Member

@lbeaufort lbeaufort commented Sep 15, 2020

Summary (required)

Use primary-key only count for Schedules B and E

Reviewers

  • Two developers please

How to test the changes locally

  • Print out the query under get_query_plan in webservices.common.counts.py (Shortcut: subl webservices/common/counts.py:38)
  • Do a Schedule B query, make sure it only selects sub_id column instead of many columns.
  • Do a Schedule E query, make sure it only selects sub_id column instead of many columns.
    Example
SELECT disclosure.fec_fitem_sched_b.sub_id AS disclosure_fec_fitem_sched_b_sub_id 
FROM disclosure.fec_fitem_sched_b

Impacted areas of the application

List general components of the application that this PR will affect:

  • /schedules/schedule_b/ endpoint count
  • /schedules/schedule_e/ endpoint count

Related PRs

List related PRs against other branches:

branch PR
feature/4368-modify-count-logic Add flag ('use_pk_for_count') to only select primary key column for count query

@lbeaufort lbeaufort force-pushed the feature/4429-use-pk-count-for-itemized branch from 42a70de to 57ff3da Compare September 15, 2020 16:05
@lbeaufort lbeaufort assigned pkfec, fec-jli and hcaofec and unassigned pkfec, fec-jli and hcaofec Sep 15, 2020
@lbeaufort lbeaufort changed the title [WIP] Use primary-key only count for Schedules B and E Use primary-key only count for Schedules B and E Sep 15, 2020
Copy link
Contributor

@jason-upchurch jason-upchurch left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @lbeaufort -- looks good. Do you want to add anything to verify the use_pk_for_count to test_counts.py? I'm sure it's fine, but it probably couldn't hurt to add a simple automated test. I saw that the PR and the develop returned the same counts.

@lbeaufort
Copy link
Member Author

Thanks for your work on this @lbeaufort -- looks good. Do you want to add anything to verify the use_pk_for_count to test_counts.py? I'm sure it's fine, but it probably couldn't hurt to add a simple automated test. I saw that the PR and the develop returned the same counts.

Thanks for your review, @jason-upchurch! This PR #4380 added an automated test and has some explanation of the testing approach.

@lbeaufort lbeaufort self-assigned this Sep 17, 2020
@jason-upchurch
Copy link
Contributor

Thanks for your work on this @lbeaufort -- looks good. Do you want to add anything to verify the use_pk_for_count to test_counts.py? I'm sure it's fine, but it probably couldn't hurt to add a simple automated test. I saw that the PR and the develop returned the same counts.

Thanks for your review, @jason-upchurch! This PR #4380 added an automated test and has some explanation of the testing approach.

Thank you @lbeaufort!

@hcaofec
Copy link
Contributor

hcaofec commented Sep 18, 2020

Download the feature branch, and tested as instructed. The queries for counting is working as expected. All tests are passed successfully. Good job!

SELECT disclosure.fec_fitem_sched_b.sub_id AS disclosure_fec_fitem_sched_b_sub_id FROM disclosure.fec_fitem_sched_b WHERE disclosure.fec_fitem_sched_b.cmte_tp IN (%(cmte_tp_1)s) AND disclosure.fec_fitem_sched_b.two_year_transaction_period IN (%(two_year_transaction_period_1)s)

SELECT ofec_sched_e_mv.sub_id AS ofec_sched_e_mv_sub_id FROM ofec_sched_e_mv WHERE get_cycle(ofec_sched_e_mv.rpt_yr) IN (%(get_cycle_1)s)

Copy link
Contributor

@hcaofec hcaofec left a comment

Choose a reason for hiding this comment

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

Look good, so approved.

@hcaofec hcaofec merged commit 013d98a into develop Sep 18, 2020
@hcaofec hcaofec deleted the feature/4429-use-pk-count-for-itemized branch September 18, 2020 12:57
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.

Implement primary key-only count for other endpoints
5 participants