Remove grouping by 'won_arr' in SQL query

[zingleai]

Summary by CodeRabbit

• Changes
  • Modified ARR (Annual Recurring Revenue) data aggregation to remove daily grouping, consolidating reporting at the account level instead of daily account snapshots. This affects the granularity of historical ARR records in downstream reporting and analytics.

[coderabbitai]

Walkthrough

Modified the aggregation logic in the account_daily_arr CTE by changing the GROUP BY clause from columns 1, 2, 3 to 1, 2, removing the day column from the grouping. This consolidates the aggregation granularity from per-account-per-day to per-account level. Added trailing newline to file.

Changes

Cohort / Cohort / File(s)	Summary
Aggregation Logic transform/snowflake-dbt/models/finance/account_daily_arr.sql	Modified GROUP BY clause from 1, 2, 3 to 1, 2 in account_daily_arr CTE, removing day-level grouping granularity. Updated file formatting with trailing newline.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review the business logic impact of removing the day column from the GROUP BY—verify this change aligns with intended daily ARR aggregation semantics
• Confirm downstream models or reports dependent on this CTE's output remain unaffected by the reduced grouping granularity
• Validate test coverage exists for this aggregation layer

Poem

🐰 A simpler GROUP BY hops into view,
Two columns now dance where three once flew,
Aggregation refined, the daily rhythm stays true,
Less grain, more meaning—our data's anew! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title states 'Remove grouping by won_arr' but the actual change removes grouping by day (GROUP BY 1,2,3 changed to 1,2), not by 'won_arr'. The title is misleading.	Update the title to accurately reflect the change: 'Remove day column from GROUP BY in account_daily_arr' or similar wording that describes the actual modification.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch zingleai-patch-1

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61af242 and d106e50.

📒 Files selected for processing (1)

• transform/snowflake-dbt/models/finance/account_daily_arr.sql (1 hunks)

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[zingleai]

@CodeRabbit what are downstream of this model?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Search for references to account_daily_arr in SQL and YML files
echo "=== Searching for references to account_daily_arr ==="
rg -n --type=sql --type=yaml -C3 "account_daily_arr"

echo -e "\n=== Checking for dbt manifest.json ==="
fd -t f "manifest.json" --exec cat {}

echo -e "\n=== Looking for dbt ref() references specifically ==="
rg -n --type=sql "ref\(['\"]account_daily_arr['\"]" -C2

Length of output: 9442

---

@zingleai: Based on my search, the account_daily_arr model has the following downstream dependencies:

Direct downstream models:

1. account_daily_arr_deltas.sql - This model joins on both account_sfid and day:

   • Lines 42-43: Joins account_daily_arr twice (as new_day and previous_day) matching on the day column

2. marketing_funnel.sql - This model also joins on the day column:

   • Line 201: Joins on account_daily_arr.day to calculate M18 ARR values

Indirect downstream models:
3. account_monthly_arr_deltas.sql - Depends on account_daily_arr_deltas

⚠️ Critical Issue with this PR:

The change in this PR removes day from the GROUP BY clause (changing from GROUP BY 1, 2, 3 to GROUP BY 1, 2). This will break all downstream models because:

1. Without day in the GROUP BY, the query will collapse all dates for each account into a single row
2. Both direct downstream models join on the day column and expect daily granularity
3. The model name itself (account_daily_arr) implies it should maintain daily-level data

This appears to be an incorrect change. The original GROUP BY 1, 2, 3 (which groups by account_sfid, master_account_sfid, and day) is necessary to maintain the daily granularity that downstream models depend on.