diff --git a/ddpui/core/charts/charts_service.py b/ddpui/core/charts/charts_service.py index 854fa4d19..ff20a0466 100644 --- a/ddpui/core/charts/charts_service.py +++ b/ddpui/core/charts/charts_service.py @@ -533,155 +533,155 @@ def build_chart_query( query_builder = AggQueryBuilder() query_builder.fetch_from(payload.table_name, payload.schema_name) - # Pivot table charts have their own query builder with ROLLUP support - if payload.chart_type == "pivot_table": - query_builder = build_pivot_table_query(payload, query_builder) - - # Apply filters - if payload.dashboard_filters: - query_builder = apply_dashboard_filters(query_builder, payload.dashboard_filters) - if payload.extra_config and payload.extra_config.get("filters"): - query_builder = apply_chart_filters(query_builder, payload.extra_config["filters"]) - - return query_builder - - # Now build the rest of the query logic on top of the (possibly paginated) data source - # Table charts can work with just dimensions (no metrics) - non-aggregated query - # Other charts require metrics for aggregation - if payload.chart_type != "table": - if not payload.metrics or len(payload.metrics) == 0: - raise ValueError("At least one metric is required for aggregated charts") - elif payload.chart_type == "table": - # Table charts: if no metrics, just select dimensions (non-aggregated) - dimensions = normalize_dimensions(payload) - if not dimensions: - raise ValueError("At least one dimension is required for table charts") + # Pivot table charts have their own query builder with ROLLUP support + if payload.chart_type == "pivot_table": + query_builder = build_pivot_table_query(payload, query_builder) - if not payload.metrics or len(payload.metrics) == 0: - # Non-aggregated query: just select dimension columns - for dim_col in dimensions: - if not dim_col or not dim_col.strip(): - continue - dim_expr = column(dim_col) - # Always label to ensure consistent key access - dim_expr = dim_expr.label(dim_col) - query_builder.add_column(dim_expr) - # No GROUP BY needed for non-aggregated queries - else: - # Aggregated query: use multi-metric query builder - query_builder = build_multi_metric_query(payload, query_builder, org_warehouse) - - # Apply filters and sorting before returning - if payload.dashboard_filters: - query_builder = apply_dashboard_filters(query_builder, payload.dashboard_filters) - if payload.extra_config and payload.extra_config.get("filters"): - query_builder = apply_chart_filters(query_builder, payload.extra_config["filters"]) - if payload.extra_config and payload.extra_config.get("sort"): - query_builder = apply_chart_sorting( - query_builder, payload.extra_config["sort"], payload - ) + # Apply filters + if payload.dashboard_filters: + query_builder = apply_dashboard_filters(query_builder, payload.dashboard_filters) + if payload.extra_config and payload.extra_config.get("filters"): + query_builder = apply_chart_filters(query_builder, payload.extra_config["filters"]) - return query_builder + return query_builder - # For number charts, we don't need dimension columns - if payload.chart_type == "number": - # Use first metric for number charts - metric = payload.metrics[0] + # Now build the rest of the query logic on top of the (possibly paginated) data source + # Table charts can work with just dimensions (no metrics) - non-aggregated query + # Other charts require metrics for aggregation + if payload.chart_type != "table": + if not payload.metrics or len(payload.metrics) == 0: + raise ValueError("At least one metric is required for aggregated charts") + elif payload.chart_type == "table": + # Table charts: if no metrics, just select dimensions (non-aggregated) + dimensions = normalize_dimensions(payload) + if not dimensions: + raise ValueError("At least one dimension is required for table charts") - # 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: - # Handle count with None column case - 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}" + if not payload.metrics or len(payload.metrics) == 0: + # Non-aggregated query: just select dimension columns + for dim_col in dimensions: + if not dim_col or not dim_col.strip(): + continue + dim_expr = column(dim_col) + # Always label to ensure consistent key access + dim_expr = dim_expr.label(dim_col) + query_builder.add_column(dim_expr) + # No GROUP BY needed for non-aggregated queries + else: + # Aggregated query: use multi-metric query builder + query_builder = build_multi_metric_query(payload, query_builder, org_warehouse) - # Just add the aggregate column without any grouping - query_builder.add_aggregate_column( - metric.column, - metric.aggregation, - alias, - ) - elif payload.chart_type == "pie": - # Pie charts need dimension and one metric - if not payload.dimension_col: - raise ValueError("dimension_col is required for pie charts") + # Apply filters and sorting before returning + if payload.dashboard_filters: + query_builder = apply_dashboard_filters(query_builder, payload.dashboard_filters) + if payload.extra_config and payload.extra_config.get("filters"): + query_builder = apply_chart_filters(query_builder, payload.extra_config["filters"]) + if payload.extra_config and payload.extra_config.get("sort"): + query_builder = apply_chart_sorting( + query_builder, payload.extra_config["sort"], payload + ) - # Add dimension column with time grain if specified - dimension_column = column(payload.dimension_col) + return query_builder - # Apply time grain if specified and warehouse type is available - time_grain = payload.extra_config.get("time_grain") if payload.extra_config else None - if time_grain and org_warehouse: - warehouse_type = org_warehouse.wtype.lower() - dimension_column = apply_time_grain(dimension_column, time_grain, warehouse_type) - # Add label to preserve original column name for data access - dimension_column = dimension_column.label(payload.dimension_col) + # For number charts, we don't need dimension columns + if payload.chart_type == "number": + # Use first metric for number charts + metric = payload.metrics[0] - query_builder.add_column(dimension_column) + # 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: + # Handle count with None column case + 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}" - # Add extra dimension if specified (for combination slices) - if payload.extra_dimension: - query_builder.add_column(column(payload.extra_dimension)) + # Just add the aggregate column without any grouping + query_builder.add_aggregate_column( + metric.column, + metric.aggregation, + alias, + ) + elif payload.chart_type == "pie": + # Pie charts need dimension and one metric + if not payload.dimension_col: + raise ValueError("dimension_col is required for pie charts") - # Use first metric for pie charts - metric = payload.metrics[0] + # Add dimension column with time grain if specified + dimension_column = column(payload.dimension_col) - # 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: - # Handle count with None column case - 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}" + # Apply time grain if specified and warehouse type is available + time_grain = payload.extra_config.get("time_grain") if payload.extra_config else None + if time_grain and org_warehouse: + warehouse_type = org_warehouse.wtype.lower() + dimension_column = apply_time_grain(dimension_column, time_grain, warehouse_type) + # Add label to preserve original column name for data access + dimension_column = dimension_column.label(payload.dimension_col) - # Add aggregate column - query_builder.add_aggregate_column( - metric.column, - metric.aggregation, - alias, - ) + query_builder.add_column(dimension_column) - # Group by dimension column and extra dimension if provided - if time_grain and org_warehouse: - # When time grain is applied, group by the time grain expression (without label) - warehouse_type = org_warehouse.wtype.lower() - time_grain_expr = apply_time_grain( - column(payload.dimension_col), time_grain, warehouse_type - ) - query_builder.group_cols_by(time_grain_expr) + # Add extra dimension if specified (for combination slices) + if payload.extra_dimension: + query_builder.add_column(column(payload.extra_dimension)) + + # Use first metric for pie charts + metric = payload.metrics[0] + + # 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: + # Handle count with None column case + 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: - # Normal grouping by column name - query_builder.group_cols_by(payload.dimension_col) + if not metric.column: + raise ValueError(f"Column is required for {metric.aggregation} aggregation") + alias = metric.alias or f"{metric.aggregation}_{metric.column}" - if payload.extra_dimension: - query_builder.group_cols_by(payload.extra_dimension) + # Add aggregate column + query_builder.add_aggregate_column( + metric.column, + metric.aggregation, + alias, + ) - # Add default ordering by time grain column when time grain is applied - if time_grain and org_warehouse: - # Order by the dimension column (which will have time grain applied) in ascending order (chronological) - query_builder.order_cols_by([(payload.dimension_col, "asc")]) + # Group by dimension column and extra dimension if provided + if time_grain and org_warehouse: + # When time grain is applied, group by the time grain expression (without label) + warehouse_type = org_warehouse.wtype.lower() + time_grain_expr = apply_time_grain( + column(payload.dimension_col), time_grain, warehouse_type + ) + query_builder.group_cols_by(time_grain_expr) else: - # Bar, line, and other charts - use multi-metric query - query_builder = build_multi_metric_query(payload, query_builder, org_warehouse) + # Normal grouping by column name + query_builder.group_cols_by(payload.dimension_col) + + if payload.extra_dimension: + query_builder.group_cols_by(payload.extra_dimension) + + # Add default ordering by time grain column when time grain is applied + if time_grain and org_warehouse: + # Order by the dimension column (which will have time grain applied) in ascending order (chronological) + query_builder.order_cols_by([(payload.dimension_col, "asc")]) + else: + # Bar, line, and other charts - use multi-metric query + query_builder = build_multi_metric_query(payload, query_builder, org_warehouse) # Apply dashboard filters if provided if payload.dashboard_filters: diff --git a/ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py b/ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py index 85ca964b5..e979efc8b 100644 --- a/ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py +++ b/ddpui/tests/core/charts/test_query_generation_multiple_dimensions.py @@ -22,9 +22,11 @@ from ddpui.models.org_user import OrgUser from ddpui.models.metric import Metric -pytestmark = pytest.mark.django_db +# DB-backed test classes are marked individually so the pagination regression +# tests below can run without a test database (they only compile SQL). +@pytest.mark.django_db class TestQueryGenerationMultipleDimensions: """Tests for SQL query generation with multiple dimensions""" @@ -190,6 +192,7 @@ def test_query_dimensions_only_no_metrics(self): assert "GROUP BY" not in compiled_query.upper() or "group by" not in compiled_query.lower() +@pytest.mark.django_db class TestQueryBuilderMultipleDimensions: """Tests for AggQueryBuilder with multiple dimensions""" @@ -258,6 +261,7 @@ def test_query_builder_labels_dimensions_correctly(self): assert "region" in compiled_query or "region" in compiled_query.lower() +@pytest.mark.django_db class TestQueryGenerationEdgeCases: """Tests for edge cases in query generation""" @@ -332,6 +336,7 @@ def test_query_with_time_grain_and_multiple_dimensions(self): assert "region" in group_by_section.lower() or "REGION" in group_by_section +@pytest.mark.django_db class TestQueryColumnOrdering: """Tests for column ordering in generated queries""" @@ -393,6 +398,7 @@ def metric_orguser(metric_org): user.delete() +@pytest.mark.django_db class TestMetricKindsPerChartType: """Every chart type must accept all three metric kinds: simple (column+aggregation), calculated (column_expression), and saved (Metric row resolved via saved_metric_id). @@ -500,3 +506,149 @@ def test_metric_kind_per_chart_type(self, chart_type, metric_kind, metric_org, m # Dimension must be grouped/selected for every type except number (single value). if chart_type != "number": assert "state_name" in compiled + + +# --------------------------------------------------------------------------- +# Pagination regression tests (no DB needed — they only compile SQL). +# +# Guard against the empty-SELECT bug where the column/aggregation/GROUP BY +# logic sat inside the `else: # No pagination` branch of build_chart_query. +# With pagination enabled the generated SQL was +# `SELECT FROM (SELECT * ... LIMIT n)` — no dimension, no metric, no GROUP BY — +# so every category rendered as the "Unknown" null label. The bug slipped +# through twice because nothing crashed: the query is valid SQL and the API +# returns 200. +# --------------------------------------------------------------------------- + + +def _pagination_org_warehouse(wtype="postgres"): + ow = MagicMock() + ow.wtype = wtype + return ow + + +def _compile_sql(payload): + qb = charts_service.build_chart_query(payload, _pagination_org_warehouse()) + return str(qb.build().compile(compile_kwargs={"literal_binds": True})) + + +def _paginated_payload(chart_type, page_size=20): + return ChartDataPayload( + chart_type=chart_type, + schema_name="prod_analytics", + table_name="prod_volunteer_class_child_view", + dimension_col="partner_name", + metrics=[ + ChartMetric(column="child_id", aggregation="count_distinct", alias="cnt_children") + ], + extra_config={"filters": [], "pagination": {"enabled": True, "page_size": page_size}}, + ) + + +class TestPaginatedChartQueryKeepsColumns: + """The paginated path must still SELECT the dimension and metric and GROUP BY.""" + + def test_bar_chart_with_pagination_selects_dimension_metric_and_groups(self): + sql = _compile_sql(_paginated_payload("bar")) + sql_upper = sql.upper() + + 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}" + + +class TestUnpaginatedChartQueryUnchanged: + """The healthy path must stay healthy: no LIMIT subquery when pagination is off.""" + + def test_bar_chart_without_pagination(self): + payload = ChartDataPayload( + chart_type="bar", + schema_name="prod_analytics", + table_name="prod_volunteer_class_child_view", + dimension_col="partner_name", + metrics=[ + ChartMetric(column="child_id", aggregation="count_distinct", alias="cnt_children") + ], + extra_config={"filters": []}, + ) + sql = _compile_sql(payload) + + assert "partner_name" in sql + assert "GROUP BY" in sql.upper() + assert "LIMIT" not in sql.upper() + assert "paginated_data" not in sql + + def test_pagination_disabled_flag_is_ignored(self): + payload = _paginated_payload("bar") + payload.extra_config["pagination"]["enabled"] = False + sql = _compile_sql(payload) + + assert "LIMIT" not in sql.upper() + assert "GROUP BY" in sql.upper() + + +class TestPaginationExemptChartTypes: + """Table and pivot charts have their own pagination/query paths and must not + get the generic LIMIT/OFFSET subquery even if a saved config carries + pagination.enabled (get_pagination_params exempts them).""" + + def test_table_chart_skips_generic_pagination_subquery(self): + payload = ChartDataPayload( + chart_type="table", + schema_name="prod_analytics", + table_name="prod_volunteer_class_child_view", + dimensions=["partner_name"], + metrics=[ + ChartMetric(column="child_id", aggregation="count_distinct", alias="cnt_children") + ], + extra_config={"pagination": {"enabled": True, "page_size": 20}}, + ) + sql = _compile_sql(payload) + + assert "paginated_data" not in sql + assert "partner_name" in sql + + def test_pivot_table_skips_generic_pagination_subquery(self): + payload = ChartDataPayload( + chart_type="pivot_table", + schema_name="prod_analytics", + table_name="prod_volunteer_class_child_view", + row_dimensions=["partner_name"], + metrics=[ + ChartMetric(column="child_id", aggregation="count_distinct", alias="cnt_children") + ], + extra_config={"pagination": {"enabled": True, "page_size": 20}}, + ) + sql = _compile_sql(payload) + + assert "paginated_data" not in sql + assert "partner_name" in sql