🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering #32883

cobobrien · 2023-11-28T17:37:34Z

What

The incremental sync for Shopify Inventory Level substreams pulls a fraction of the data that it should. This is due to the combination of an unsortable Parent Stream (Location) and the functionality fo the filter_records_newer_than_state method. Detailed explanation with examples in the issue: #32817

How

Adds Incremental Inventory Levels by using the updated_at_min filter. Is far more performant than what existed previously due to no longer requiring the filter_records_newer_than_state functionality.

Previously incremental substreams pulls had to query all existing data for that resource without any filtering, and then filter out the older-than-state data after the fact in the connector runtime (via the filter_records_newer_than_state method). This way, only the newer-than-state data is pulled via the REST interface.

It runs in a fraction of the time of the old implementation and will pull all qualifying records

🚨 User Impact 🚨

All qualifying Inventory Level records will be pulled. This incremental stream sync will also be much more performant than it currently is.

Pre-merge Actions

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

vercel · 2023-11-28T17:37:40Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Dec 4, 2023 4:32pm

github-actions · 2023-11-28T17:38:00Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

…bstream---Incremental-Sync-Flawed-Filtering

marcosmarxm

Left some comments thanks for the contribution @cobobrien

airbyte-integrations/connectors/source-shopify/source_shopify/source.py

marcosmarxm · 2023-12-04T16:24:56Z

airbyte-integrations/connectors/source-shopify/source_shopify/source.py

@@ -408,7 +408,7 @@ def stream_slices(self, stream_state: Mapping[str, Any] = None, **kwargs) -> Ite
                # avoid checking `deleted` records for substreams, a.k.a `Metafields` streams,
                # since `deleted` records are not available, thus we avoid HTTP-400 errors.
                if self.deleted_cursor_field not in record:
-                    yield {self.slice_key: record[self.nested_record]}
+                    yield {self.slice_key: record[self.nested_record]} | (stream_state or {})


Can you explain the reason of returning stream_state here?

I required access to the original updated_at date (from the state file passed in as an argument to the sync execution) in the request_params method. I couldn't find a better approach:

The stream_state in the request_params seems to be always an empty dict

self.get_updated_state does no good here as we're looking for the value to use for the updated_at_min param

This connector does not have a self.state property

I'm eager to know of a better way of achieving this as it felt a bit hacky to me too. Thanks @marcosmarxm 🙌

…source.py Co-authored-by: Marcos Marx <[email protected]>

cobobrien · 2023-12-08T12:48:05Z

Left some comments thanks for the contribution @cobobrien

@marcosmarxm I have updated this PR. Could I please get a review?

bazarnov · 2023-12-08T16:00:07Z

@cobobrien I'll take a look.

bazarnov · 2023-12-11T13:46:24Z

@cobobrien Thank you for your time spent on the problem, currently, I don't feel this PR would be needed in the light of this one: #32345 which will migrate the InventoryLevels from REST to BULK and will fix the state issues as well.

@marcosmarxm Should we still merge this in with what is being said? Overall the PR looks good.

bazarnov · 2024-02-26T18:24:20Z

The fix has been merged @marcosmarxm FYI.

marcosmarxm · 2024-02-26T18:30:05Z

@cobobrien can you try latest version of the connector?

marcosmarxm · 2024-04-11T13:11:10Z

Closed as #32345 solves the issue.

cobobrien added 2 commits November 28, 2023 17:02

Inventory Level incremental substream fix

9eeba58

dockerImageTag bump

61569f7

octavia-squidington-iii added area/connectors Connector related issues community connectors/source/shopify labels Nov 28, 2023

Update the ReadMe

3c6e1dd

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 28, 2023

Merge branch 'master' into cobobrien/32817-Shopify-Inventory-Level-Su…

2b2f7a6

…bstream---Incremental-Sync-Flawed-Filtering

cobobrien changed the title ~~(32817) Fix: Shopify Inventory Level Substream - Incremental Sync Flawed Filtering~~ (32817) Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering Nov 28, 2023

vercel bot deployed to Preview November 28, 2023 17:44 View deployment

cobobrien changed the title ~~(32817) Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering~~ Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering Nov 28, 2023

cobobrien changed the title ~~Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering~~ 🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering Nov 28, 2023

marcosmarxm requested a review from bazarnov November 29, 2023 13:58

marcosmarxm added gl gl-review labels Nov 29, 2023

oustynova added team/connectors-python and removed gl labels Nov 29, 2023

marcosmarxm requested changes Dec 4, 2023

View reviewed changes

Update airbyte-integrations/connectors/source-shopify/source_shopify/…

eb18217

…source.py Co-authored-by: Marcos Marx <[email protected]>

cobobrien requested a review from marcosmarxm December 4, 2023 16:57

marcosmarxm added certified gl labels Feb 1, 2024

lazebnyi assigned bazarnov Feb 7, 2024

bazarnov mentioned this pull request Feb 7, 2024

🐛 Source Shopify: Migrate from REST > GraphQL BULK Operations where possible, fixed STATE collisions for sub-streams #32345

Merged

marcosmarxm closed this Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering #32883

🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering #32883

cobobrien commented Nov 28, 2023 •

edited

Loading

vercel bot commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023

marcosmarxm left a comment

marcosmarxm Dec 4, 2023

cobobrien Dec 4, 2023

cobobrien commented Dec 8, 2023

bazarnov commented Dec 8, 2023

bazarnov commented Dec 11, 2023

bazarnov commented Feb 26, 2024

marcosmarxm commented Feb 26, 2024

marcosmarxm commented Apr 11, 2024

🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering #32883

🐛 Source Shopify: Fix Inventory Level Substream - Incremental Sync Flawed Filtering #32883

Conversation

cobobrien commented Nov 28, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

vercel bot commented Nov 28, 2023 • edited Loading

github-actions bot commented Nov 28, 2023

Before Merging a Connector Pull Request

marcosmarxm left a comment

Choose a reason for hiding this comment

marcosmarxm Dec 4, 2023

Choose a reason for hiding this comment

cobobrien Dec 4, 2023

Choose a reason for hiding this comment

cobobrien commented Dec 8, 2023

bazarnov commented Dec 8, 2023

bazarnov commented Dec 11, 2023

bazarnov commented Feb 26, 2024

marcosmarxm commented Feb 26, 2024

marcosmarxm commented Apr 11, 2024

cobobrien commented Nov 28, 2023 •

edited

Loading

vercel bot commented Nov 28, 2023 •

edited

Loading