reports: a count tile reads its number from the server (#7161) - #7236
Merged
Conversation
Two follow-ups to #7102, where a dashboard `kind: count` tile over an AGGREGATING report learned to sum the report's `count(*)` measure - the rows being groups, the count endpoint reports the number of groups. The sum was the BROWSER's: the reports store fetched every group row of the report, per tile, per dashboard load, to add up one column - thousands of rows for a report grouped by day or customer, where the endpoint it replaced was one number in one round trip. The generated report repository now sums the column in SQL (`SELECT SUM("<alias>") FROM (<the filtered report query>)`) behind a `POST /sum`, carrying the same per-column conditions the tile's `at` pins and the report page's filter panel already speak, and the store reads that. The column is matched against the report's own aliases and must be numeric - the same allowlist discipline the conditions use, so a total over a text column or an alias the report does not carry is a 400, not a 500 - and a report yielding no rows sums to zero rather than null. The second half is the editor: it offered ANY `aggregate: COUNT` column as the Count column, including a `count(<field>)`, whose sum is the number of rows carrying a value in that field and never the record count. The intent path was protected by `getCountMeasure()`'s count(*)-only rule; the hand-authored path now is too - only a `count(*)` column is offered, and a stored one that is not is dropped on load (with a console warning and the file marked dirty) rather than left naming a column the picker no longer shows. Fixes #7161 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Two follow-ups to #7102, where a dashboard
kind: counttile over an aggregating report learned to sum the report'scount(*)measure - the rows being groups, the count endpoint reports the number of groups.The sum was the browser's
application-core's reports store fetched every group row of the report, per tile, per dashboard load, to add up one column - thousands of rows for a report grouped by day or customer, where the/countendpoint it replaced was one number in one round trip.The generated report repository now sums the column in SQL -
SELECT SUM("<alias>") AS "REPORT_SUM" FROM (<the filtered report query>)- behind aPOST /sum, carrying the same per-columnconditionsthe tile'satpins and the report page's filter panel already speak, and the store reads that.The column is matched against the report's own aliases (the allowlist the conditions already use) and must be numeric, so a total over a text column or an alias the report does not carry is a 400, not a 500; a report yielding no rows sums to zero rather than null.
The editor offered a column that cannot be the record count
It offered ANY
aggregate: COUNTcolumn, including acount(<field>), whose sum is the number of rows carrying a value in that field - a different number, and never the record count. The intent path was protected bygetCountMeasure()'scount(*)-only rule; the hand-authored path is now too: only acount(*)column is offered, and a stored one that is not is dropped on load (console warning, file marked dirty) rather than left naming a column the picker no longer shows.Verification
IntentEmissionCoverageIT- emission (the SQL wrapper, the numeric-type gate, the controller's@Post("/sum")) plus the live half against the published app: theClaimsByUnitreport groups two claims into one row, so/sumanswers 2 where/countanswers 1 - the two numbers differ, which is what makes the assertion about reading the right one; a text column and an unknown alias are 400s; and a pinned condition narrows the sum to 0.Ran green locally with one pre-existing assertion relaxed: master is currently red on
IntentEmissionCoverageIT:2000(a master being deleted must suspend the per-line totals write-back, #7143) on both DB legs - it fails on run 34337194456 too, and it aborts this single big test method before any of the assertions above are reached. Nothing in this PR touches that path.Docs: dirigible-io/dirigible-io.github.io#241.
Fixes #7161