Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Dec 12, 2025

fix(SUPPORT-14107): fix sync and async extraction for action breakdown queries

Summary

Fixes two bugs in the Facebook Ads V2 extractor that caused sync and async extraction to fail for action breakdown queries:

Bug 1 - Sync Extraction: DSL-style queries like insights.level(ad).action_breakdowns(action_type).date_preset(last_3d)...{fields} were not being parsed correctly. The _build_params method only extracted metric, period, since, until but missed level, action_breakdowns, date_preset, time_increment, breakdowns, and the fields list from curly braces. This caused the API request to be sent without critical parameters, returning minimal/incorrect data.

Bug 2 - Async Extraction: Action breakdown queries returned correct row counts but empty metric columns (ad_name, clicks, impressions, spend, reach). The _copy_common_fields method only copied ID fields to action rows, not the metric fields from the original row.

Bug 3 - Detection Logic: Action breakdown detection only checked query.parameters but DSL-style queries have action_breakdowns in the fields string, causing incorrect parsing behavior.

Updates since last revision

Backwards compatibility fix #1: Initial testing revealed that adding too many new columns to _copy_common_fields caused "column mismatch" errors with existing customer tables. The field list has been reduced to only include columns that already existed in V2 async tables: ad_name, impressions, clicks, spend, reach.

Backwards compatibility fix #2: Testing on job 151760765 revealed a "missing columns: account_id" error. The issue was that when parsing the DSL {fields} list, we now explicitly set params["fields"], but the old behavior relied on Facebook API defaults which included account_id. Fixed by automatically appending account_id to the fields list when parsing DSL syntax, ensuring existing tables continue to receive this structural field.

Review & Testing Checklist for Human

  • Test with existing customer tables - Run against project 15170 with existing data to verify no column mismatch errors occur. Previous versions failed on job 151760765 due to missing account_id.
  • Verify sync extraction row count - Sync extraction was returning 11 rows instead of ~19,590. Confirm the fix produces the expected row count.
  • Test both sync and async modes - Both query types (27613 sync, 28043 async) should work with existing tables
  • Verify DSL parsing edge cases - The string splitting approach (split(".level(")[1].split(")")[0]) could break with malformed input or nested parentheses
  • Compare output with V1 extractor - Verify the output schema (columns, PK, row count) matches what V1 produces for the same query

Recommended Test Plan

  1. In project 15170 dev branch, run the sync query: insights.level(ad).action_breakdowns(action_type).date_preset(last_3d).time_increment(1){ad_id,ad_name,campaign_id,campaign_name,cost_per_action_type,actions,impressions,reach,clicks,spend}
  2. Verify output has ~19,590 rows with correct PK: parent_id, campaign_id, date_start, date_stop, ads_action_name, action_type, ad_id
  3. Run the async query and verify metric columns (ad_name, clicks, impressions, spend, reach) are populated
  4. Important: Test with existing tables (not empty storage) to verify backwards compatibility - the account_id column should now be present

Notes

…n queries

- Extend _build_params to parse DSL syntax: level, action_breakdowns,
  date_preset, time_increment, breakdowns, and fields list from curly braces
- Extend action breakdown detection to check both parameters and fields string
  (for DSL-style queries like insights.action_breakdowns(action_type))
- Extend _copy_common_fields to include metric fields (ad_name, impressions,
  clicks, spend, reach, etc.) for action breakdown queries

Co-Authored-By: Matyáš Jirát <[email protected]>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

devin-ai-integration bot and others added 2 commits December 12, 2025 11:27
Remove extra metric fields (adset_name, frequency, cpc, cpm, cpp, ctr) from
_copy_common_fields to maintain backwards compatibility with existing V2 tables.
Only copy fields that already existed in V2 async tables per SUPPORT-14107:
ad_name, impressions, clicks, spend, reach.

Co-Authored-By: Matyáš Jirát <[email protected]>
Add account_id to the fields list when parsing DSL syntax to maintain
backwards compatibility with existing tables. Previously, when no explicit
fields list was set, the Facebook API returned account_id by default.
Now that we parse the DSL fields list, we need to explicitly include it.

Co-Authored-By: Matyáš Jirát <[email protected]>
devin-ai-integration bot added a commit that referenced this pull request Dec 17, 2025
Combines fixes from both PRs:
- PR #21 (SUPPORT-14412): Instagram accounts token handling, error handling, 30-day validation
- PR #22 (SUPPORT-14107): DSL parameter extraction for action breakdown queries

Resolves merge conflict in page_loader.py by including both:
1. DSL parameter extraction (level, action_breakdowns, date_preset, time_increment, breakdowns, fields list)
2. Instagram insights 30-day date range validation

Co-Authored-By: Matyáš Jirát <[email protected]>
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.

1 participant