Skip to content

Add core_excellent_cohort_revenue model #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zingleai wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add core_excellent_cohort_revenue model #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions dbt/models/marts/core/core_excellent_cohort_revenue.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
-- Total revenue summary for products in the Excellent rating cohort (avg_rating >= 4.5)
-- Filters dim_products to rating_tier = 'Excellent' and aggregates revenue,
-- profit, units sold, and margin metrics at the cohort level.
Comment on lines +1 to +3
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing {{ config(materialized='table') }} — intent not enforced.

The PR explicitly states this model should use table materialization, but no {{ config() }} block exists. Without it, dbt uses whatever default is set in dbt_project.yml for this path (marts/core). If that default is view or ephemeral, the model will silently behave differently from the stated intent, impacting downstream query performance and dependency expectations.

🛠️ Proposed fix — add config block 
+{{ config(materialized='table') }}
+
 -- Total revenue summary for products in the Excellent rating cohort (avg_rating >= 4.5)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@dbt/models/marts/core/core_excellent_cohort_revenue.sql` around lines 1 - 3,
Add a dbt config block to explicitly set materialization to table so the model
core_excellent_cohort_revenue uses table materialization regardless of project
defaults; insert a top-of-file line calling the Jinja config helper ({{
config(materialized='table') }}) in core_excellent_cohort_revenue.sql so the
model behavior matches the PR intent and downstream performance expectations.

⚠️ Potential issue | 🟠 Major

Missing {{ config(materialized='table') }} — stated intent not enforced.

The PR explicitly targets table materialization, but no {{ config() }} block is present. Without it, dbt resolves materialization from dbt_project.yml defaults for this path. If that default is view or ephemeral, the model silently behaves differently from the stated intent, impacting downstream query performance and dependency contracts.

🛠️ Proposed fix — add config block 
+{{ config(materialized='table') }}
+
 -- Total revenue summary for products in the Excellent rating cohort (avg_rating >= 4.5)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
-- Total revenue summary for products in the Excellent rating cohort (avg_rating >= 4.5)
-- Filters dim_products to rating_tier = 'Excellent' and aggregates revenue,
-- profit, units sold, and margin metrics at the cohort level.
{{ config(materialized='table') }}
-- Total revenue summary for products in the Excellent rating cohort (avg_rating >= 4.5)
-- Filters dim_products to rating_tier = 'Excellent' and aggregates revenue,
-- profit, units sold, and margin metrics at the cohort level.
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@dbt/models/marts/core/core_excellent_cohort_revenue.sql` around lines 1 - 3,
Add an explicit dbt config block to enforce table materialization for the model
in core_excellent_cohort_revenue.sql: insert a top-of-file "{{
config(materialized='table') }}" config (before any SQL or comments) so the
model uses table materialization regardless of dbt_project.yml defaults and
matches the PR intent; ensure the config appears above the SELECT/CTE logic in
the existing model file.

with source as (

select
product_id,
product_name,
product_category,
avg_rating,
rating_tier,
total_units_sold,
product_total_revenue,
product_gross_profit,
margin_pct
from {{ ref('dim_products') }}
where rating_tier = 'Excellent'

),

final as (

select
rating_tier,
count(product_id) as product_count,
sum(product_total_revenue) as total_revenue,
sum(product_gross_profit) as total_gross_profit,
sum(total_units_sold) as total_units_sold,
round(avg(avg_rating)::numeric, 2) as avg_rating,
round(avg(margin_pct)::numeric, 4) as avg_margin_pct
Comment on lines +29 to +30
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

avg(margin_pct) is a simple arithmetic mean — consider a revenue-weighted margin instead.

A simple average of per-product margin percentages gives equal weight to a $10-revenue product and a $10M-revenue product. For a cohort revenue model, the standard approach is a weighted average derived directly from the already-aggregated sums:

total_gross_profit / NULLIF(total_revenue, 0)

This is more analytically sound and avoids a division-by-zero risk in the process.

♻️ Proposed fix — replace simple avg with revenue-weighted margin 
         round(avg(avg_rating)::numeric, 2)  as avg_rating,
-        round(avg(margin_pct)::numeric, 4)  as avg_margin_pct
+        round(
+            (sum(product_gross_profit) / nullif(sum(product_total_revenue), 0))::numeric,
+            4
+        )                                   as avg_margin_pct
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@dbt/models/marts/core/core_excellent_cohort_revenue.sql` around lines 29 -
30, Replace the unweighted avg(margin_pct) with a revenue-weighted margin
computed as total_gross_profit / NULLIF(total_revenue, 0) so large-revenue items
carry appropriate weight and avoid divide-by-zero; update the column currently
aliased as avg_margin_pct to use this expression (with the same rounding/casting
as other aggregates, e.g., round((total_gross_profit::numeric /
NULLIF(total_revenue, 0))::numeric, 4) as avg_margin_pct) and ensure
total_gross_profit and total_revenue are available in the same SELECT scope
where avg_margin_pct is produced.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

avg(margin_pct) is a simple arithmetic mean — use a revenue-weighted margin instead.

A simple average of per-product margin percentages assigns equal weight to every product regardless of its revenue contribution. For a cohort revenue model this produces a misleading metric. The revenue-weighted equivalent is already computable from the aggregates already in this CTE:

sum(product_gross_profit) / nullif(sum(product_total_revenue), 0)

This also avoids a potential division-by-zero if total_revenue is zero.

♻️ Proposed fix — replace simple avg with revenue-weighted margin 
-        round(avg(margin_pct)::numeric, 4)  as avg_margin_pct
+        round(
+            (sum(product_gross_profit) / nullif(sum(product_total_revenue), 0))::numeric,
+            4
+        )                                   as avg_margin_pct
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@dbt/models/marts/core/core_excellent_cohort_revenue.sql` at line 30, Replace
the simple arithmetic average used to compute avg_margin_pct with a
revenue-weighted margin: instead of using avg(margin_pct) for the avg_margin_pct
alias, compute sum(product_gross_profit) divided by
nullif(sum(product_total_revenue), 0) (and then round/cast as numeric to 4
decimals) so the metric weights by revenue and avoids division-by-zero; update
the expression that defines avg_margin_pct in this CTE accordingly (referencing
avg_margin_pct, margin_pct, product_gross_profit, and product_total_revenue).

from source
group by rating_tier

)

select * from final