Skip to content

TDL-19486 Add limit clause to core stream queries - #68

Merged
dsprayberry merged 15 commits into
mainfrom
TDL-19486-add-limit-clause-to-core-stream-queries
Jul 5, 2022
Merged

TDL-19486 Add limit clause to core stream queries#68
dsprayberry merged 15 commits into
mainfrom
TDL-19486-add-limit-clause-to-core-stream-queries

Conversation

@prijendev

@prijendev prijendev commented Jun 17, 2022

Copy link
Copy Markdown
Contributor

Description of change

  • Added support of configurable page limit parameter into the tap.
  • LIMIT parameter is added for all core streams except the below streams,
    • call_details
    • campaign_lables
    • ad_group_criterion
    • campaign_criterion
  • LIMIT clauses in the ad_group_criterion and campaign_criterion(stream which has composite primary keys) may result in the infinite loop.
    • For example, the limit is 10. campaign_criterion stream has a total of 20 records with campaign_id = 1 . So, in the first call, the tap retrieves 10 records, and the next time query would look like the below,
      WHERE campaign_id >= 1
    • Now, the tap will again fetch records with campaign_id = 1. That's why we should not pass the LIMIT clause in the query of these streams.
    • We can not do queries like WHERE pk_1 > 1 OR (pk_1 = 1 and pk_2 > 1) because google ads do not support OR parameter. We had a slack conversation about the same.
  • Added unit test case to validate different values of the query_limit parameter
  • Added integration test case.

QA steps

  • automated tests passing
  • manual qa steps passing

Risks

Rollback steps

  • revert this branch

@prijendev prijendev changed the title Initial commit for add page limit. Add limit clause to core stream queries Jun 17, 2022
Comment thread tap_google_ads/sync.py Outdated
@prijendev
prijendev requested a review from NevilParikh14 June 17, 2022 12:06
@prijendev prijendev changed the title Add limit clause to core stream queries TDL-19486 Add limit clause to core stream queries Jun 17, 2022
Comment thread tap_google_ads/streams.py Outdated
Comment thread tap_google_ads/sync.py Outdated
Comment thread tap_google_ads/sync.py Outdated
Comment thread tap_google_ads/sync.py Outdated
@prijendev
prijendev requested a review from dsprayberry June 20, 2022 10:22
Comment thread tests/test_google_ads_page_limit.py Outdated
Comment thread tests/test_google_ads_page_limit.py Outdated
"limit": value
}

class TestQueryLimitParam(unittest.TestCase):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are good test cases.

@prijendev
prijendev requested a review from kspeer825 June 22, 2022 13:00
@dsprayberry dsprayberry mentioned this pull request Jun 30, 2022
2 tasks
@dsprayberry
dsprayberry merged commit 2b9d9d2 into main Jul 5, 2022
@dsprayberry
dsprayberry deleted the TDL-19486-add-limit-clause-to-core-stream-queries branch July 5, 2022 13:03
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.

6 participants