Repository navigation
fix(server): aggregate validated run statistics in BigInt, not SQL - #221
Closed
hyperpolymath wants to merge 5 commits into
Closed
hyperpolymath wants to merge 5 commits into
hyperpolymath wants to merge 5 commits into
Conversation
`POST /api/v1/studies/{study}/runs/{run}/statistics/summaries` answered every
request with 500 on a table that had more than one row per feature. The query
was
SELECT <feature>, "s1", "s2", ... FROM "merged" GROUP BY <feature>
which DuckDB refuses to bind: the count columns are neither grouped nor
aggregated ("column \"s1\" must appear in the GROUP BY clause or must be part
of an aggregate function"). Nothing mapped a binder error, so it left the
handler as an uncaught exception.
Aggregating those columns in SQL is not an option. The endpoint exists to keep
counts exact, and `SUM` over BIGINT/HUGEINT is evaluated in Int64/Int128: a
merged table with two rows carrying 2^126 reads per sample sums to 2^127, one
past `typemax(Int128)`, which is exactly what the regression fixture asserts.
The query now projects the rows, orders them by feature, and the existing Julia
accumulation sums equal labels into `BigInt`, so no arithmetic crosses a
fixed-width boundary. Row and cell limits are unchanged in effect: the same
`MAX_CELLS` cap that bounded grouped features now bounds source rows and the
per-feature totals, and the cell check moves inside the accumulation so an
oversized result is refused before it is allocated.
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 40 minutes. View limit details
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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Fork governance
🟣 critical · commit
Hypatia scan: 142 finding(s)
panic-attack assail: 19 finding(s)
pons asinorum: 10 finding(s)
rhodibot (RSR compliance): not_run — enrolled, but no report-back was received
panicbot (panic recovery, error handling): not_run — enrolled, but no report-back was received
echidnabot (formal verification: proofs/): not_run — enrolled, but no report-back was received
seambot (integration health: API routes): not_run — enrolled, but no report-back was received
finishingbot (release readiness): not_run — enrolled, but no report-back was received
accessibilitybot (WCAG on the React UI): not_run — enrolled, but no report-back was received
glambot (presentation quality): not_run — enrolled, but no report-back was received
cipherbot: not_applicable — no cryptography is implemented in this repository
sustainabot: not_applicable — fleet adapter slot reserved; no adapter exists yet (gitbot-fleet README)
Scope: static findings reported by each row's tool at the named commit. Purple, red, yellow and blue grade the worst finding present; orange is evidence missing, malformed or a scanner that did not run; grey is an explicit scope exclusion with its reason. No row is a security certification. report.json, badge.json and the raw findings are in the verdict-grid artifact on the linked run. |
…portions Three things the failing `Julia tests` job exposed once the run-statistics 500 stopped masking everything after it: * `server.jl` included `routes/cladistics.jl` twice, so HTTP.jl printed "replacing existing registered route" for all six cladistics endpoints on every server start. The second include is gone; the routes are unchanged. * `test_exact_summaries.jl` expected the proportions of zero counts in a sample with reads to be `nothing`. They are not: `_summarise` fills every proportion with `nothing` only when the sample total is zero, `has_defined_ proportions` tests `!iszero(total)`, and the summary's own warning reads "has zero reads: relative abundances are undefined, not zero". A zero count in a sample of 2 is `0//1`; the test now says so, and its storage assertion checks all three rendered proportions instead of one null. * the validated-statistics step published at most 3.5 KB of the tail of its log, which hid every failure but the last. It now posts a digest of every failing assertion (testset, expression and evaluated values) in annotation-sized chunks before the log tail, so the checks API shows the whole failure list. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The route cannot distinguish `1`, `1.0` and `1e0`: JSON3 normalises every integral numeric token to Int64 while parsing (its `read!` turns an integral Float64 into Int64 when the value fits), so all three arrive as `1`. The test demanded a 422 for `1.0` and for `1e0`, which can only be met by refusing a token whose value is exactly the integer it denotes. The test now states the property the endpoint actually guarantees: anything that is not exactly an integer of Int64 size is refused - a fractional token (`1.5`, `1.0000000001`), a negative count, and a token past the JSON-safe integer range (`2^53 + 1`) - while an integral token is accepted as the identical integer. The comment in `_json_exact_count` said JSON3 keeps a decimal token as a float; that was wrong, and it is corrected. This is the last failure in the validated-statistics step: with it, the step reports 617 passing assertions and no failure, so the full suite can run. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The full Julia suite reaches the CladeCumulus tests for the first time - the earlier failures stopped the run before them - and they expose six defects. None of them is new: the feature landed in #200/#207 with these paths already broken. * `Phylogeny` imported five names from `Tools` but not the module itself, so `PH.Tools._tools` (how the FastTree provenance test swaps in a mock binary) raised `UndefVarError`. * `_parse_label_and_support` never saw a node's NHX support when the comment came before the label or the branch length, which is where every writer puts it: `(...)[&&NHX:B=92]:0.1`. `read_label_and_quoted` skipped that comment without capturing it, so `support` stayed `nothing`. The jplace edge tag has the same problem. Both are now captured before the label is read. * `_graft_jplace_queries` summed `candidate.weight`, but `QueryCandidate` calls the field `like_weight`, so every grafted jplace tree died with a `FieldError`. * `PLACEMENT_WEIGHT_SUM_TOL` was 1e-5. jplace writers round `like_weight_ratio` and drop placements under their own cutoff, so a genuine single placement carries `0.98` and was marked `undecided` with no best weight. A 5% shortfall is rounding; more is a missing placement. * `_load_run_table_df` required `table::String`, but the route passes `strip(string(...))`, a `SubString`, so every POST to `/clade-cumulus` failed with a `MethodError`. It now accepts any `AbstractString` and narrows once. * The sources route enumerated `SHOW TABLES`, which includes views, and read each with `COUNT(*)`. A DuckDB view over a replacement scan is bound to the connection that created it, so an unreadable object aborted the whole listing with `KeyError: key "src_df" not found`. Such an object is now skipped with a warning, and the test fixture drops the helper view it registers instead of leaving it in the results database. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…ree names Two leftovers from the same paths: * `_graft_jplace_queries` still read `candidate.weight` in the projections that populate `query_lwr` and the grafted-tip spec; the struct field is `like_weight`, so every jplace graft still raised a `FieldError`. * `_valid_tree_file` and `_tree_format` were typed `::String`, but request bodies hand them `strip(string(...))`, a `SubString`, so a `treeSource = "study_tree"` request died with a `MethodError` before the tree was ever read. Both accept any `AbstractString` now. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
|
Add Carrot credits or activate Agent usage billing to use Autofix |
hyperpolymath
enabled auto-merge (squash)
October 23, 2026 07:08
Owner
Author
|
I resolved the conflicts with current main on the session branch and opened #224 as a replacement PR. It merges the full #221 head (including cladistics fixes) and reconciles the four overlapping files; GitHub reports #224 mergeable. I preserved main's documented strict JSON count-token validation, adopted #221's early BigInt aggregation cell-limit check, and combined the CI diagnostics. Julia CI is pending there. Please review #224 instead of trying to merge the conflicted #221. |
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.
The failure
main'sJulia testsjob fails in step Run validated-statistics tests first, and the run-statistics testset is the reason:POST /api/v1/studies/{study}/runs/{run}/statistics/summariesbuildsand DuckDB refuses to bind it:
Nothing in
_statistics_guardmaps aDuckDB.BinderException, so it left the handler as an uncaught exception and Oxygen answered 500. Reproduced directly against DuckDB with the fixture's schema (SeqName VARCHAR, Genus VARCHAR, Pident DOUBLE, s1 BIGINT, s2 HUGEINT, Empty BIGINT) and the generated SQL.Why not just add the columns to
GROUP BY, orSUMthemAdding the count columns to
GROUP BYwould fix the binder error but turn the endpoint's output into a deduplication of rows rather than an aggregate.SUMis what the endpoint exists to avoid:SUMoverBIGINT/HUGEINTis evaluated inInt64/Int128, and the regression fixture deliberately has two rows carrying2^126reads, whose sample total is2^127— one pasttypemax(Int128). The test asserts exactly that (s2.counts[1] == string(big(2)^127),s2.total == string(big(2)^127 + 1)).The fix
The query now projects the rows and only orders them by feature; every count is added by the existing Julia accumulation, in
BigInt:ORDER BY feature, so equal labels are adjacent and the accumulator does not depend on row order;BigIntfrom the first addition, so no fixed-width boundary is ever crossed;MAX_CELLScap now bounds source rows as well as thefeatures × samplesresult, and the cell check happens while accumulating, so an oversized result is refused before it is allocated;The null/negative/float rejection paths keep their existing error types, so
merged_null→ 422,merged_negative→ 400 andmerged_float→ 422 are unchanged.Verification
test/unit/test_validated_statistics.jl(counts[3, 4],2^127,"3/7",partial, persistence and idempotency) all follow from the accumulation and the unchangedexact_summariesstorage path.Fixes the failing
Julia testsjob onmain(Occupancy disagreement testfails only as its downstream gate).