Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10607 +/- ##
=======================================
Coverage 58.03% 58.03%
=======================================
Files 215 215
Lines 16683 16683
Branches 3357 3357
=======================================
Hits 9682 9682
Misses 6511 6511
Partials 490 490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Updated documentation to use 1.0 * instead of 100.0 * for percentage calculations - Frontend now automatically multiplies by 100 when format: percent is used - Changed examples in getting started guides, recipes, and API documentation - Updated Funnels extension conversionsPercent measure - Updated test fixtures and example schemas - Kept decimal point multiplication (1.0 *) to ensure proper floating point division Resolves CUB-2081 Co-authored-by: Artyom Keydunov <artyom.keydunov@gmail.com>
493b641 to
89658f0
Compare
|
Claude finished @keydunov's task in 4m 32s —— View job Code Review
SummaryThe core rationale of this PR is correct. I verified that The Funnels.ts change and all documentation updates are correct and consistent. Issues Found1. Precision loss in
|
| File | Line | Measure |
|---|---|---|
test/integration/postgres/calendars.test.ts |
119 | completed_percentage |
test/integration/postgres/multi-stage.test.ts |
63 | return_rate |
test/integration/postgres/calc-groups.test.ts |
139 | return_rate |
test/integration/postgres/member-expression.test.ts |
97 | return_rate |
These are inline schema definitions (not from the updated calendar_orders.yml fixture), so they were easy to miss.
What Looks Good
- Funnels.ts (
packages/cubejs-schema-compiler/src/extensions/Funnels.ts:48): The only production code change is correct. TheCASE WHEN ... THEN 1.0 * conversions / firstStepConversions ELSE NULL ENDproperly handles division-by-zero and ensures float division. - Test fixture (
calendar_orders.yml): Updated correctly and also normalized the SQL expression style from({completed_count} / NULLIF({count}, 0)) * 100.0to1.0 * {completed_count} / NULLIF({count}, 0)— cleaner. - Documentation: All 13 doc files are updated consistently across both
docs/anddocs-mintlify/trees, covering YAML and JavaScript variants. - Examples: Both recipe examples updated correctly.
- No security concerns: Changes are purely arithmetic/formatting.
- No snapshot tests broken: Verified no snapshot files reference the changed measures.
Verdict
The PR is solid and the fix is correct. The two minor issues above (ROUND precision and missed test files) are not blockers but worth addressing for completeness.
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | ||
| CUBE.total_orders | ||
| }::NUMERIC * 100.0, 2)` | ||
| }::NUMERIC, 2)` |
There was a problem hiding this comment.
Potential precision loss: With the old code, ROUND(ratio * 100.0, 2) gave 2 decimal places in the percentage space (e.g., 33.33). Now ROUND(ratio, 2) rounds the raw ratio to 2 decimal places (e.g., 0.33), and d3's .2% format will display it as 33.00% instead of 33.33%.
Since the format: percent (d3 .2%) already handles rounding when displaying, ROUND is arguably unnecessary now. But if you want to keep it for clean SQL output, consider increasing the precision:
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | |
| CUBE.total_orders | |
| }::NUMERIC * 100.0, 2)` | |
| }::NUMERIC, 2)` | |
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | |
| CUBE.total_orders | |
| }::NUMERIC, 4)` |
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | ||
| CUBE.total_orders | ||
| }::NUMERIC * 100.0, 2)` | ||
| }::NUMERIC, 2)` |
There was a problem hiding this comment.
Same precision concern as the mintlify version — ROUND(ratio, 2) truncates to 2 decimal places before the frontend multiplies by 100, so 0.3333 becomes 0.33 → displayed as 33.00% instead of 33.33%.
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | |
| CUBE.total_orders | |
| }::NUMERIC * 100.0, 2)` | |
| }::NUMERIC, 2)` | |
| `ROUND(${CUBE[`total_${status}_orders`]}::NUMERIC / ${ | |
| CUBE.total_orders | |
| }::NUMERIC, 4)` |
| title: `Percentage of ${status} orders`, | ||
| sql: (CUBE) => | ||
| `ROUND(${CUBE[`Total_${status}_orders`]}::numeric / ${CUBE.totalOrders}::numeric * 100.0, 2)`, | ||
| `ROUND(${CUBE[`Total_${status}_orders`]}::numeric / ${CUBE.totalOrders}::numeric, 2)`, |
There was a problem hiding this comment.
Same precision concern as the docs: ROUND(ratio, 2) pre-truncates before the frontend's ×100 formatting. A raw ratio of 0.3333 becomes 0.33 → displayed as 33.00% rather than 33.33%. Consider ROUND(..., 4) to preserve two visible decimal places after the frontend multiplies, or remove the ROUND entirely and let the format: percent (d3 .2%) handle all rounding.
| `ROUND(${CUBE[`Total_${status}_orders`]}::numeric / ${CUBE.totalOrders}::numeric, 2)`, | |
| `ROUND(${CUBE[`Total_${status}_orders`]}::numeric / ${CUBE.totalOrders}::numeric, 4)`, |
Description
This PR removes incorrect AI instructions and examples that suggest multiplying percentage metrics by 100. The Cube frontend now automatically handles percentage formatting when
format: percentis specified, so multiplication by 100 in the SQL is no longer needed and results in incorrect values (e.g., showing 5000% instead of 50%).Changes
Documentation Updates
completed_percentageexamples in Cloud and Databricks tutorialspaying_percentageCode Changes
conversionsPercentmeasure to use1.0 *instead of100.0 *examples/recipes/active-users: Fixed WAU to MAU ratioexamples/recipes/referencing-dynamic-measures: Fixed percentage measurescalendar_orders.ymltest fixtureKey Changes
Before (Incorrect):
After (Correct):
Rationale
format: percent1.0 *ensures proper floating-point division while avoiding the multiplication issueTesting
Related Issue
Resolves CUB-2081
Linear Issue: CUB-2081