chore(data-warehouse): Disabled the geographic_stats table by default for google ads#51984
Open
chore(data-warehouse): Disabled the geographic_stats table by default for google ads#51984
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a per-table “default sync enabled” flag to the external data source schema flow, and uses it to disable the (potentially massive) Google Ads geographic_stats table by default when creating new Google Ads sources.
Changes:
- Added
should_sync_defaulttoSourceSchemaand included it in theexternal_data_sources/database_schemaAPI response. - Extended Google Ads schema generation to propagate
descriptionandshould_sync_defaultfrom the Google Ads schema registry. - Updated the data warehouse source wizard to initialize
should_syncbased onshould_sync_default.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| products/data_warehouse/backend/api/external_data_source.py | Adds should_sync_default to the database_schema API response payload. |
| posthog/temporal/data_imports/sources/google_ads/source.py | Propagates table description and should_sync_default into SourceSchema. |
| posthog/temporal/data_imports/sources/google_ads/schemas.py | Sets geographic_stats to not sync by default and adds a warning description. |
| posthog/temporal/data_imports/sources/google_ads/google_ads.py | Threads should_sync_default/description from RESOURCE_SCHEMAS into table schema objects. |
| posthog/temporal/data_imports/sources/common/schema.py | Extends SourceSchema with should_sync_default (defaulting to True). |
| frontend/src/types.ts | Adds should_sync_default to ExternalDataSourceSyncSchema. |
| frontend/src/scenes/data-warehouse/new/sourceWizardLogic.tsx | Uses should_sync_default when initializing schema sync selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/temporal/data_imports/sources/google_ads/schemas.py
Line: 638
Comment:
**Grammar error in description string**
`"unless if"` is grammatically incorrect — it should be just `"unless"` (or `"if"`).
```suggestion
"description": "This can load 100's of millions of rows for active campaigns. We don't recommend enabling this table unless you know you need it.",
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/types.ts
Line: 5531
Comment:
**Type inconsistency with the defensive `??` fallback**
`should_sync_default` is typed as a non-optional `boolean`, but the consuming code in `sourceWizardLogic.tsx` uses `schema.should_sync_default ?? true`, which implies the value could be `undefined` at runtime (e.g. from a cached or older API response). These two are inconsistent: either the type should be optional (`boolean | undefined` or `should_sync_default?: boolean`) to reflect the defensive coding, or the `?? true` fallback in the logic is superfluous and can be dropped.
```suggestion
should_sync_default?: boolean
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge branch 'master' into tom/default-g..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Size Change: +531 B (0%) Total Size: 110 MB ℹ️ View Unchanged
|
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Changes
Dont have it enabled by default for new google ads sources