fix: Do no aggregate results when metrics=[]

[Vitor-Avila]

SUMMARY

The AG Grid un-aggregated table is sending metrics=[] to the backend when a CSV download is triggered from the dashboard, which makes sense (the un-aggregated chart doesn't have any metrics). In the backend, we validate if metrics is not None to decide if a GROUP BY should be added, in this case incorrectly evaluating to True.

As a consequence, the CSV file would only include distinct rows.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

1. Create an un-aggregated AG Grid table using the cleaned_sales_data dataset.
2. Use deal_size as a column.
3. Save the chart and add it to a dashboard.
4. Access the dashboard.
5. Trigger a CSV download.
6. Validate the CSV includes the same amount of rows returned by the chart.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #3286b4

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: b434be4..b434be4

  • superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts
  • superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts
• Files skipped - 0
• Tools

  • Eslint (Linter) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[Vitor-Avila]

Hey @alexandrusoare it would be good to get your thoughts on this one since you worked on the original PR. Thanks!

[alexandrusoare]

LGTM

[Vitor-Avila]

After talking with @betodealmeida and doing some more testing here, I noticed that this is actually a lot easier to reproduce. Since the AG Grid table is always adding a GROUP BY, downloading an un-aggregated table to CSV would only include distinct rows.

We've agreed to actually take a first pass on fixing this at the backend: the backend should be capable of receiving metrics=[] and not add a group by for it.

[betodealmeida]

Yeah!

[Vitor-Avila]

@betodealmeida will have to revert to fixing this at the AG Grid level. Dashboard native filters intentionally set metrics=[] and expect an aggregation. Personally, I think this is confusing (I understand the need to be able to aggregate without any metric, but I would expect something more explicit instead of just metrics=[]), but changing this would be a larger refactor.

We used to have both groupby and columns but it was deprecated and unified in this PR, which makes sense I agree we can call both use-cases as columns. Ideally, instead of using metrics=[] we would rely on something like QueryMode, but I don't think native filters use that. Long-term, I could see native filters having their own dedicated endpoint to avoid the need to comply with the complexity of /api/v1/chart/data but that's also a larger effort.