Fix build_chart_query producing empty SELECT when pagination is enabled - #1442
Open
siddhant3030 wants to merge 6 commits into
Open
Fix build_chart_query producing empty SELECT when pagination is enabled#1442siddhant3030 wants to merge 6 commits into
siddhant3030 wants to merge 6 commits into
Conversation
Move all column-selection logic (for number, pie, bar, line, table, pivot_table chart types) out of the 'else: # No pagination' branch so it executes regardless of whether pagination is enabled. Previously, when limit was not None the if-branch only set up the paginated subquery and outer query_builder, but the chart-type column-building code was entirely inside the else block and was skipped. This resulted in a SELECT with no columns, causing 'Available keys: []' errors on chart-data-preview requests with pagination parameters. Fixes DALGO-BACKEND-2BQ (https://dalgo.sentry.io/issues/113816406/)
WalkthroughChart query construction now shares chart-type handling across paginated and non-paginated queries. Tests cover pagination, SQL structure, page sizes, disabled pagination, and table or pivot exceptions. ChangesChart query flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ddpui/core/charts/charts_service.py (1)
591-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated metric-to-column logic into a helper.
The number branch (Lines 591-613) and the pie branch (Lines 639-661) contain identical metric handling: expression metrics, count-with-null-column aliasing, and aggregate column addition.
build_pivot_table_query(Lines 465-472) implements a third variant of the same split. A shared helper keeps alias rules consistent when metric handling changes.♻️ Proposed helper extraction
def add_metric_column(query_builder: AggQueryBuilder, metric) -> AggQueryBuilder: """Add one metric to the SELECT list as an expression or an aggregate column.""" if metric.column_expression: alias = metric.alias or "expression_metric" query_builder.add_column(literal_column(metric.column_expression).label(alias)) return query_builder if metric.aggregation and metric.aggregation.lower() == "count" and metric.column is None: alias = f"count_all_{metric.alias}" if metric.alias else "count_all" else: if not metric.column: raise ValueError(f"Column is required for {metric.aggregation} aggregation") alias = metric.alias or f"{metric.aggregation}_{metric.column}" query_builder.add_aggregate_column(metric.column, metric.aggregation, alias) return query_builderThen both branches reduce to:
- # Expression metric: inline raw SQL (e.g. "SUM(a)/SUM(b)") — no aggregation/column. - if metric.column_expression: - alias = metric.alias or "expression_metric" - query_builder.add_column(literal_column(metric.column_expression).label(alias)) - else: - ... + query_builder = add_metric_column(query_builder, metric)Also applies to: 639-661
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/core/charts/charts_service.py` around lines 591 - 613, Extract the shared metric handling from the number and pie branches into an `add_metric_column` helper that accepts an `AggQueryBuilder` and metric, preserving expression metrics, count-with-null-column aliases, validation, and aggregate-column addition. Replace both duplicated branches with calls to this helper, and update the metric split in `build_pivot_table_query` to use it so all metric alias rules remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/core/charts/charts_service.py`:
- Around line 682-684: Add a compiled-query test for a paginated non-table
aggregation path, using a bar, line, or pie chart with limit set. Exercise
build_chart_query followed by build_multi_metric_query and assert the generated
query contains the expected dimension, metrics, and GROUP BY clauses.
---
Nitpick comments:
In `@ddpui/core/charts/charts_service.py`:
- Around line 591-613: Extract the shared metric handling from the number and
pie branches into an `add_metric_column` helper that accepts an
`AggQueryBuilder` and metric, preserving expression metrics,
count-with-null-column aliases, validation, and aggregate-column addition.
Replace both duplicated branches with calls to this helper, and update the
metric split in `build_pivot_table_query` to use it so all metric alias rules
remain consistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe1124a1-9418-4752-8eb1-16914bc0dd67
📒 Files selected for processing (1)
ddpui/core/charts/charts_service.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1442 +/- ##
==========================================
+ Coverage 65.74% 65.80% +0.05%
==========================================
Files 170 170
Lines 19658 19658
==========================================
+ Hits 12925 12936 +11
+ Misses 6733 6722 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Compile-only tests (no DB) asserting that build_chart_query with pagination enabled still selects the dimension and metric columns and applies GROUP BY — the empty-SELECT failure mode this PR fixes. Also pins the unpaginated path (no LIMIT subquery) and the table/pivot exemptions. Verified to fail on main before the fix (4 failures) and pass with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/tests/unit/test_chart_pagination_query.py`:
- Around line 51-71: Update the pagination query tests around _compile and the
line/pie chart test methods to extract the outer SELECT list from the generated
SQL, then assert that partner_name and the DISTINCT metric expression appear in
that list. Replace whole-query assertions for these columns while retaining the
existing GROUP BY, LIMIT, and paginated_data checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5d9fef3-b3ac-4297-a39e-b8522ad996bd
📒 Files selected for processing (1)
ddpui/tests/unit/test_chart_pagination_query.py
Comment on lines
+51
to
+71
| assert "partner_name" in sql, f"dimension column missing from SELECT:\n{sql}" | ||
| assert "distinct" in sql.lower(), f"metric aggregate missing from SELECT:\n{sql}" | ||
| assert "GROUP BY" in sql_upper, f"GROUP BY missing:\n{sql}" | ||
| assert "LIMIT 20" in sql_upper, f"pagination LIMIT missing:\n{sql}" | ||
| assert "paginated_data" in sql, f"pagination subquery missing:\n{sql}" | ||
|
|
||
| def test_line_chart_with_pagination_selects_dimension_metric_and_groups(self): | ||
| sql = _compile(_paginated_payload("line")) | ||
|
|
||
| assert "partner_name" in sql | ||
| assert "distinct" in sql.lower() | ||
| assert "GROUP BY" in sql.upper() | ||
| assert "LIMIT 20" in sql.upper() | ||
|
|
||
| def test_pie_chart_with_pagination_selects_dimension_metric_and_groups(self): | ||
| sql = _compile(_paginated_payload("pie")) | ||
|
|
||
| assert "partner_name" in sql | ||
| assert "distinct" in sql.lower() | ||
| assert "GROUP BY" in sql.upper() | ||
| assert "LIMIT 20" in sql.upper() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert columns in the outer SELECT list.
Lines 51-71 search the complete SQL statement. A query can contain partner_name only in GROUP BY and still pass, although chart results would not contain the dimension value. Extract the outer SELECT list and assert both partner_name and DISTINCT there.
Proposed test fix
def _compile(payload):
qb = build_chart_query(payload, _org_warehouse())
return str(qb.build().compile(compile_kwargs={"literal_binds": True}))
+def _outer_select_list(sql):
+ return sql.upper().split("FROM", 1)[0].replace("SELECT", "", 1).strip()
+
...
def test_bar_chart_with_pagination_selects_dimension_metric_and_groups(self):
sql = _compile(_paginated_payload("bar"))
sql_upper = sql.upper()
+ select_list = _outer_select_list(sql)
- assert "partner_name" in sql, f"dimension column missing from SELECT:\n{sql}"
- assert "distinct" in sql.lower(), f"metric aggregate missing from SELECT:\n{sql}"
+ assert "PARTNER_NAME" in select_list, f"dimension column missing from SELECT:\n{sql}"
+ assert "DISTINCT" in select_list, f"metric aggregate missing from SELECT:\n{sql}"Apply the same assertions to the line and pie tests.
Also applies to: 77-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/tests/unit/test_chart_pagination_query.py` around lines 51 - 71, Update
the pagination query tests around _compile and the line/pie chart test methods
to extract the outer SELECT list from the generated SQL, then assert that
partner_name and the DISTINCT metric expression appear in that list. Replace
whole-query assertions for these columns while retaining the existing GROUP BY,
LIMIT, and paginated_data checks.
Move the pagination tests from a standalone file into test_query_generation_multiple_dimensions.py. The module-level django_db marker becomes per-class markers on the existing classes so the new pagination classes stay DB-free (they only compile SQL). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py`:
- Around line 555-586: Update the pagination chart tests around _compile_sql and
the chart-specific test methods to extract the outer SELECT clause rather than
searching the full SQL string. Assert that this clause includes partner_name and
the expected metric alias for each bar, line, and pie chart, while retaining the
existing grouping and pagination assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4080abd3-6b14-4a8c-a656-7557594ff367
📒 Files selected for processing (1)
ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py
Comment on lines
+555
to
+586
| assert "partner_name" in sql, f"dimension column missing from SELECT:\n{sql}" | ||
| assert "distinct" in sql.lower(), f"metric aggregate missing from SELECT:\n{sql}" | ||
| assert "GROUP BY" in sql_upper, f"GROUP BY missing:\n{sql}" | ||
| assert "LIMIT 20" in sql_upper, f"pagination LIMIT missing:\n{sql}" | ||
| assert "paginated_data" in sql, f"pagination subquery missing:\n{sql}" | ||
|
|
||
| def test_line_chart_with_pagination_selects_dimension_metric_and_groups(self): | ||
| sql = _compile_sql(_paginated_payload("line")) | ||
|
|
||
| assert "partner_name" in sql | ||
| assert "distinct" in sql.lower() | ||
| assert "GROUP BY" in sql.upper() | ||
| assert "LIMIT 20" in sql.upper() | ||
|
|
||
| def test_pie_chart_with_pagination_selects_dimension_metric_and_groups(self): | ||
| sql = _compile_sql(_paginated_payload("pie")) | ||
|
|
||
| assert "partner_name" in sql | ||
| assert "distinct" in sql.lower() | ||
| assert "GROUP BY" in sql.upper() | ||
| assert "LIMIT 20" in sql.upper() | ||
|
|
||
| def test_pagination_respects_page_size(self): | ||
| sql = _compile_sql(_paginated_payload("bar", page_size=75)) | ||
| assert "LIMIT 75" in sql.upper() | ||
|
|
||
| def test_select_list_is_never_empty(self): | ||
| """The literal failure mode: 'SELECT FROM' with nothing between.""" | ||
| for chart_type in ("bar", "line", "pie"): | ||
| sql = _compile_sql(_paginated_payload(chart_type)) | ||
| select_clause = sql.upper().split("FROM")[0].replace("SELECT", "").strip() | ||
| assert select_clause, f"{chart_type}: empty SELECT list:\n{sql}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the outer SELECT list contains the dimension.
Lines 555-586 search the complete SQL string. A query that includes partner_name only in GROUP BY would pass these tests but still omit the chart category from the result rows.
Extract the outer SELECT clause. Assert that it contains partner_name and the metric alias for each chart type.
Proposed test change
def test_select_list_is_never_empty(self):
"""The literal failure mode: 'SELECT FROM' with nothing between."""
for chart_type in ("bar", "line", "pie"):
sql = _compile_sql(_paginated_payload(chart_type))
- select_clause = sql.upper().split("FROM")[0].replace("SELECT", "").strip()
+ select_clause = sql.upper().split("FROM", 1)[0]
+ assert "PARTNER_NAME" in select_clause, (
+ f"{chart_type}: dimension missing from outer SELECT:\n{sql}"
+ )
+ assert "CNT_CHILDREN" in select_clause, (
+ f"{chart_type}: metric missing from outer SELECT:\n{sql}"
+ )
+ select_clause = select_clause.replace("SELECT", "").strip()
assert select_clause, f"{chart_type}: empty SELECT list:\n{sql}"📝 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
| assert "partner_name" in sql, f"dimension column missing from SELECT:\n{sql}" | |
| assert "distinct" in sql.lower(), f"metric aggregate missing from SELECT:\n{sql}" | |
| assert "GROUP BY" in sql_upper, f"GROUP BY missing:\n{sql}" | |
| assert "LIMIT 20" in sql_upper, f"pagination LIMIT missing:\n{sql}" | |
| assert "paginated_data" in sql, f"pagination subquery missing:\n{sql}" | |
| def test_line_chart_with_pagination_selects_dimension_metric_and_groups(self): | |
| sql = _compile_sql(_paginated_payload("line")) | |
| assert "partner_name" in sql | |
| assert "distinct" in sql.lower() | |
| assert "GROUP BY" in sql.upper() | |
| assert "LIMIT 20" in sql.upper() | |
| def test_pie_chart_with_pagination_selects_dimension_metric_and_groups(self): | |
| sql = _compile_sql(_paginated_payload("pie")) | |
| assert "partner_name" in sql | |
| assert "distinct" in sql.lower() | |
| assert "GROUP BY" in sql.upper() | |
| assert "LIMIT 20" in sql.upper() | |
| def test_pagination_respects_page_size(self): | |
| sql = _compile_sql(_paginated_payload("bar", page_size=75)) | |
| assert "LIMIT 75" in sql.upper() | |
| def test_select_list_is_never_empty(self): | |
| """The literal failure mode: 'SELECT FROM' with nothing between.""" | |
| for chart_type in ("bar", "line", "pie"): | |
| sql = _compile_sql(_paginated_payload(chart_type)) | |
| select_clause = sql.upper().split("FROM")[0].replace("SELECT", "").strip() | |
| assert select_clause, f"{chart_type}: empty SELECT list:\n{sql}" | |
| assert "partner_name" in sql, f"dimension column missing from SELECT:\n{sql}" | |
| assert "distinct" in sql.lower(), f"metric aggregate missing from SELECT:\n{sql}" | |
| assert "GROUP BY" in sql_upper, f"GROUP BY missing:\n{sql}" | |
| assert "LIMIT 20" in sql_upper, f"pagination LIMIT missing:\n{sql}" | |
| assert "paginated_data" in sql, f"pagination subquery missing:\n{sql}" | |
| def test_line_chart_with_pagination_selects_dimension_metric_and_groups(self): | |
| sql = _compile_sql(_paginated_payload("line")) | |
| assert "partner_name" in sql | |
| assert "distinct" in sql.lower() | |
| assert "GROUP BY" in sql.upper() | |
| assert "LIMIT 20" in sql.upper() | |
| def test_pie_chart_with_pagination_selects_dimension_metric_and_groups(self): | |
| sql = _compile_sql(_paginated_payload("pie")) | |
| assert "partner_name" in sql | |
| assert "distinct" in sql.lower() | |
| assert "GROUP BY" in sql.upper() | |
| assert "LIMIT 20" in sql.upper() | |
| def test_pagination_respects_page_size(self): | |
| sql = _compile_sql(_paginated_payload("bar", page_size=75)) | |
| assert "LIMIT 75" in sql.upper() | |
| def test_select_list_is_never_empty(self): | |
| """The literal failure mode: 'SELECT FROM' with nothing between.""" | |
| for chart_type in ("bar", "line", "pie"): | |
| sql = _compile_sql(_paginated_payload(chart_type)) | |
| select_clause = sql.upper().split("FROM", 1)[0] | |
| assert "PARTNER_NAME" in select_clause, ( | |
| f"{chart_type}: dimension missing from outer SELECT:\n{sql}" | |
| ) | |
| assert "CNT_CHILDREN" in select_clause, ( | |
| f"{chart_type}: metric missing from outer SELECT:\n{sql}" | |
| ) | |
| select_clause = select_clause.replace("SELECT", "").strip() | |
| assert select_clause, f"{chart_type}: empty SELECT list:\n{sql}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py` around
lines 555 - 586, Update the pagination chart tests around _compile_sql and the
chart-specific test methods to extract the outer SELECT clause rather than
searching the full SQL string. Assert that this clause includes partner_name and
the expected metric alias for each bar, line, and pie chart, while retaining the
existing grouping and pagination assertions.
himanshudube97
self-requested a review
August 18, 2026 05:32
himanshudube97
approved these changes
Aug 18, 2026
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
3 participants
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
Any non-table chart (bar, line, pie) with
pagination.enabled: truein its config renders garbage: every category shows as "Unknown" and metric values are lost.Example: a bar chart of
COUNT_DISTINCT(child_id)bypartner_namewith page size 20 returnsxAxisData: ["Unknown", "Unknown", ...]— 20 bars, all labeled Unknown.Root cause
In
build_chart_query(ddpui/core/charts/charts_service.py), all column-selection logic — dimension columns, metric aggregates, andGROUP BY, for every chart type — was nested inside theelse: # No paginationbranch. With pagination enabled the function returned a query with an empty SELECT list:Postgres accepts this and returns N column-less rows. The transform step then reads the missing dimension key as
Noneon every row and maps each one to the "Unknown" null label — one "Unknown" bar per raw row, since grouping never happened.Fix
De-indent the column/aggregation/GROUP BY logic out of the
elsebranch so it runs on both the paginated and non-paginated paths. With the fix, the same payload generates:The pivot-table early-return path (which bypasses generic pagination) is preserved.
Verification
ddpui/tests/core/charts/,test_pivot_query_builder.py,test_pivot_transform.py(109 passed; thedjango_db-marked tests error locally for lack of a test DB and will run in CI).Notes
main; this PR is a clean cherry-pick of the fix onto currentmain. fix: chart preview aggregation with pagination and sorting #1385 can be closed in favor of this.🤖 Generated with Claude Code
Summary by CodeRabbit