Fix: Resolve issue where PyAirbyte would fail if property names contain the dot character ('.'), e.g. with source-google-ads
#343
Conversation
Fix nested primary key not supported bug
WalkthroughWalkthroughThe recent changes introduce a normalization step for primary keys in the Changes
Assessment against linked issues
Would you like to add more details to the Walkthrough or Changes sections, or do you think they capture everything needed? Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@aaronsteers Can you please look into this PR once you have some bandwidth? |
|
/test-pr
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
airbyte/shared/catalog_providers.py (1)
153-154: Normalization step for primary keysThe changes introduce a normalization step using
LowerCaseNormalizerto ensure that primary key components are consistently formatted before concatenation. This is a good addition as it can prevent issues related to case sensitivity when comparing keys. 👍Just a thought - are there any other normalization steps that might be needed for primary keys in your use case? For example, removing special characters or whitespace. Wdyt?
tests/integration_tests/fixtures/source-test/source_test/run.py
Outdated
Show resolved
Hide resolved
tests/integration_tests/fixtures/source-test/source_test/run.py
Outdated
Show resolved
Hide resolved
|
/test-pr |
|
@Udit107710 and @sukantaroy01 - Thanks for your work on this. Do you have the ability to enable commits from maintainers/reviewers? (Looks like it is not enabled as of now.) |
|
Opened second PR targeting this one: |
|
Thanks @aaronsteers Merged your PR! |
|
@sukantaroy01 - This looks good and I really appreciate your investment here. When I ran on the other PR, there were still some failing tests, mostly related to listing the stream names in the source-test fixture, and similar trivial changes that broke existing tests. That said, this PR can't accept my contributions so I will go ahead and merge, to resolve the small test failures in a subsequent PR which I can tackle fairly quickly. Thanks very much for contributing to PyAirbyte! 🎉 ❤️ |
aaronsteers
changed the title
'.'), e.g. with source-google-ads
Fixes: #181
During a proof of concept (POC) with the Google Ads source, I encountered an issue where primary keys containing periods (e.g., campaign.id, adset.id) were not being correctly handled. The root cause was that get_primary_keys did not normalize column names before checking for nested keys.
To address this, I implemented normalization of primary keys prior to the nested key check. This resolved the issue, allowing successful data collection from the Google Ads platform.
Summary by CodeRabbit
.gitignorefile to exclude PyCharm project files for a cleaner repository.