Repository navigation
1.9.79: a report breakdown says when it has more rows than it returned - #284
Open
tracking202 wants to merge 5 commits into
Open
tracking202 wants to merge 5 commits into
tracking202 wants to merge 5 commits into
Conversation
GET /reports/breakdown answered 50 rows unless limit asked for more (500 at most) and said nothing of the rest, so every reader of one page read it as the whole breakdown. It now reads one row past the page and answers truncated, limit and offset beside data, as GET /reports/timeseries says truncated. No total: a count of the groups is a second aggregation of the window, measured at about 60% of the page's own query (300,000 clicks over 60,000 keywords, MariaDB 10.11); COUNT(*) OVER () cost nothing there but is unmeasured on MySQL 8.0 and has no row to carry it on a page past the end; and the readers only decide whether to read on. The other list reports in ReportsController are complete (daypart, weekpart, rotator stats), refuse past their cap (groups) or already say truncated (timeseries). The CLI: - report breakdown and analytics pass the keys through under --json, stamp truncated on every --ndjson line (as masked), and under a table, CSV or --quiet print one stderr note naming the next --offset; the client-side filters are said to see only the page. bin/p202 report:breakdown prints the same note. - report breakeven takes --limit and --offset and reads one page, as report breakdown does, ranked by margin within it, and says the same. - report losers, report winners and campaign optimize read one page of 50 and classified or ranked only those (measured live: losers listed 48 of 650 losing keywords, $4.80 of $37.15 spend). They read page after page, 500 a request, up to the 5000 rows with the most clicks, and say truncated: true when the answer covers only those. - report crosstab ranked the first 50 rows by clicks: the server ranks them by --metric over every row now (one more than --limit-rows, to know whether more remain), each row's columns are read in full up to 5000, and truncated (with a note) says what was left out. - campaign optimize no longer drops a failed read of the other campaigns. - The MCP tools report_breakdown, report_breakeven and report_crosstab say in their description what truncated: true means. TestEveryBreakdownReadSaysWhetherRowsRemain holds every read of reports/breakdown in go-cli to a pager or to a one-page answer that prints breakdownMoreRowsNote. A new agent eval case (report-depth-004) counts 60 keywords through the paging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr
# Conflicts: # 202-config/changelogs.json
# Conflicts: # 202-config/changelogs.json
The paragraphs the breakdown change added to the help of report breakeven, losers, winners and crosstab lengthened the long descriptions p202 search ranks on, and after #270 that moved "set the api url" from config set-url to system integrations (TestSearchReadsEditVerbsAsUpdate: the two scored 0.0328 and 0.0323 on 1.9.79, 0.0325 each with the paragraphs). One line each says the same, and the two score as on 1.9.79 again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Contributor
Brings in 1.9.79's new commits. The conflicts were in 202-config/changelogs.json, changelogs.txt, where both sides added lines: the incoming lines are kept as they are and this branch's follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr
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
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.
Backlog: TX1Q
What and why
GET /reports/breakdownanswered 50 rows unlesslimitasked for more (500 at most), and its answer held onlydata,breakdownandavailable_breakdowns: nothing said rows remained. Every reader of one page read it as the whole breakdown. Measured on a live instance with 654 keyword rows:p202 report losers --breakdown keywordlisted 48 losing keywords costing $4.80, of 650 costing $37.15.Server
LIMIT limit+1) and answerstruncated(true when rows remain),limitandoffsetbesidedata, the spelling and shapeGET /reports/timeseriesalready uses fortruncated/limit.offsetis added because a breakdown pages; the next page isoffset + limit.COUNT(*) OVER ()in the same query cost nothing measurable there (7.3 s), but it is unmeasured on MySQL 8.0 (CI's server), and a page past the end has no row to carry it, so it would need the second query exactly where it is asked for. Every reader we have decides only whether to read on, whichtruncatedanswers.ReportsControllerdo not share the shape: daypart (24 rows) and weekpart (7) are complete, rotator stats lists every rule, groups refuses past 5000 rather than cutting, and timeseries already saystruncated.Go CLI (and MCP)
Rule for the signal:
--jsonpasses the API's keys through;--ndjsonstamps"truncated": trueon every line (asmaskedis, ininternal/output, soreport timeseries --ndjsoncarries it too); a table,--csvor--quietgets one note on stderr naming the flag that reads more;--json/--ndjsonkeep stderr for errors, as they do formasked. stdout stays data.report breakdown,analyticsNote: more rows remain after rows 1-50; --offset 50 reads the next page (--limit takes up to 500).--min-clicks/--min-cost/--zero-leads/--havingfilter only the page, and the note says so (the filtered answer now keeps the server's other keys)report breakeven--limit/--offset, ranked by margin within the page, saystruncatedreport losers,report winners--jsonalways hastruncatedreport crosstab--metricover every row (sort=<metric>, one more than--limit-rows); columns: every value up to 5000;truncated+ a note naming--limit-rowsor the boundcampaign optimizetruncatedoffers, _ :=, CLAUDE.md #1) and answered "no alternative": it fails now, naming itanalytics --split-at,list --with-statsfetchAllRowsPaged)fetchBreakdownRowsis the pager: it asks for one row more than it still needs, so a server from beforetruncatedis answered too (an exact 5000 is told from more by one extra request). The MCP toolsreport_breakdown,report_breakevenandreport_crosstabsay in their description whattruncated: truemeans (kept short: with 1.9.79 merged the catalogue is 84,700 characters of its 85,000 budget, 307 more than 1.9.79;report_winners/report_loserscarry the field without a note, since few reach 5000 rows).bin/p202 report:breakdownprints the same note under its table.Machine check
TestEveryBreakdownReadSaysWhetherRowsRemain(go-cli) walks every non-test Go file and holds each call naming"reports/breakdown"to a pager (fetchAllRowsPaged/fetchAllPages),fetchBreakdownRowsitself, or a function that also callsbreakdownMoreRowsNote. It does not see an endpoint held in a variable.Agent eval
report-depth-004-a-keyword-count-reads-every-page: setup fires 60 keywords, the ask counts them; the reference agent pages untiltruncatedis false. Run on 10/23 (see Live below).Verification
.claude/skills/p202-verify/scripts/verify.sh --since origin/1.9.79 --changed, on 5413e6a (1.9.79 merged at 915a367; schema tier on the scratch databasep202_tx1q_it;P202_FAKE_GOOGLE_PORT=8281, since the Play Integrity test's fixed port 8241 collided with another worktree's suite in an earlier run here):unit: 6108 tests, 5 skipped (the 4 documented
UserTestDB-singleton skips, andCronEntryPointsFailLoudlyTest, which skips because the live instance's installer wrote a202-config.phpinto this worktree; it is not committed).BreakdownTruncationIntegrationTestis@group integrationand outside the schema tier: run by hand againstp202_tx1q_itafter it, OK (4 tests, 25 assertions).After merging 1.9.79,
TestSearchReadsEditVerbsAsUpdatefailed: the help paragraphs this change added lengthened the long descriptionsp202 searchranks on, and "set the api url" moved fromconfig set-url(0.0328 on 1.9.79) to a tie withsystem integrations(0.0325 each). 5413e6a cuts each paragraph to one line; both score as on 1.9.79 again.Tests added:
ReportsControllerTest(more rows than the limit, exactly the limit at 7/50/500, every page of 120 rows, past the end),BreakdownTruncationIntegrationTest(@group integration, real MariaDB: 51 keywords, the default page, a page of exactly the rows, paging 3 × 17 where the last full page is the last, a sorted page), Goreport_breakdown_truncation_test.go(fake servers with and without the flag, every command above, every output mode),output_test.go(NDJSON stamp),mcp_test.go,ReportBreakdownCommandTest.Plants, each restored and checked with
cmp, each caught by a named test (not a build failure): the server bindinglimitnotlimit+1,>=for>, the slice dropped (3 PHP unit + 3 integration tests); in Go, analytics' note removed, the pager reading one page, the old-server exact-count branch, the NDJSON stamp, the filter dropping the answer's keys, crosstab ranking client-side, the note printed under--json, campaign optimize dropping the error, breakeven dropping the page keys, a new one-page read with no note (the structural test), losers dropping the flag, the MCP note removed.Live, in two parts (instance installed from this branch on 10/23, MariaDB 10.11, PHP 8.3; 749 clicks over 654 keywords fired through dl.php and dej.php run, 714 keywords after the eval case's 60).
On 10/23, on 1dd3663/b5c598d4 (before 1.9.79's #270, #280, #281, #282 and the help trim), with the licence stub up, every CLI path below ran end to end:
truncated: true;limit=500&offset=154(exactly the 500 remaining)false;offset=153true;offset=6540 rowsfalse.p202 report breakdown --breakdown keyword --table: the note on stderr, nothing on stdout;--limit 500 --offset 500: 154 rows, no note;--zero-leads: the note with the filter sentence;--json: 50 rows,truncated/limit/offset, empty stderr;--ndjson: 50 lines each with"truncated":true, empty stderr.p202 analytics --group-by keyword --table: the note.p202 report breakeven --aff-campaign-id 2 --table: the note;--limit 500 --offset 600 --json: 54 rowstruncated: false.p202 report losers --breakdown keyword --no-attribution-check --json: 650 rows, $37.15,truncated: false(the base branch's binary on the same instance: 48 rows, $4.80, no flag).p202 report crosstab --rows ppc_account --cols keyword --metric clicks --json: 654 columns read over two pages,truncated: false.p202 mcp call report_breakdownwithlimit 5: 5 rows,truncated: true, limit: 5, offset: 0.p202 eval runofreport-depth-004with the reference agent: deterministic graders pass (needs_judgefor the rubric); with the server planted to answertruncated: falsealways it fails (reply does not contain "60", reply 50).Today, on 5413e6a, with the licence stub down (the instance re-checked its licence and the Go CLI is refused 402
licenceon every command):truncated: true,limit 50,offset 0;limit=500&offset=214(exactly the 500 remaining)false;offset=213true;offset=7140 rowsfalse;limit=20&offset=60true;limit=3&sort=total_clicks&sort_dir=ASCtrue;limit=7&period=today(no clicks today) 0 rowsfalse. Paging withlimit=500untiltruncatedis false: 2 pages, 714 rows, 714 distinct ids, 869 clicks, which is what SQL counts on202_dataengine(714 keywords, 869 clicks).php bin/p202 report:breakdown --breakdown keyword --period alltime(not licence-gated): the note on stderr and 54 table lines on stdout;--limit 500 --offset 500(214 rows, the last page): no note;--json: 50 rows,truncated: true,limit 50,offset 0, empty stderr.Not run, and why
report breakdown(table,--csv,--json,--ndjson, the filters),analytics,report breakeven,report losers,report winners,report crosstab,campaign optimize,p202 mcp calland thereport-depth-004eval case through the reference agent could not run end to end on 5413e6a. They ran on 10/23 on the same Go code bar the help-text trim (above); since then they are covered by the cmd tests against fake servers only, whichgoin the ladder ran.report_losers/report_winners/report_crosstabthat reads that far, plus the attribution check's reads, can be refused by the guard with how to ask for less. That is the guard's refusal, never a cut answer.@group instancesuites and the rest of the agent eval suite were not run.limit + 1bound.For the reviewer
report losers/winnersnow make up to 11 requests where they made 1, each a full aggregation; on a large keyword set that is slower, and through MCP a complete losers list can pass the 60,000-character limit and be refused with "ask for less" (on the live instance: 710 rows, 120k characters;min_clicks: 2brought it to 26k). Before, the same call answered the first 50 rows as all of them.breakdownReadRows) followsGROUPS_MAX_ROWS; past it the answer says what it covers and the note says to narrow the window or filter. There is no flag to raise it.--quiet);report timeseriesprints its warning in every mode, and losers'/winners' other notes print in every mode too. I followed themaskedrule and the brief rather than those.report_optimize.goare the pager, the breakevenRunEread and two flags, and the triage fetch/output lines;enrichBreakevenand the multi-profile code are untouched.Found outside this item
GET /ltv/breakdownpages withlimit/offset(50 by default) and says nothing of the rows after a page, the same gap.pagination.total, attributionmeta.groups); a sweep could land one.Generated by Claude Code