Verify engine correctness in a downstream job that reuses benchmark results - #128
Merged
Conversation
jiayuasu
changed the title
jiayuasu
force-pushed
the
feature/correctness-harness
branch
from
July 23, 2026 06:48
7c06c08 to
de366f0
Compare
jiayuasu
force-pushed
the
feature/correctness-harness
branch
from
July 23, 2026 07:24
de366f0 to
ac2d76f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a correctness-verification path to SpatialBench CI by dumping normalized per-query results during benchmark runs and comparing them downstream against committed SF1 ground-truth answers, failing CI on mismatches while avoiding duplicate benchmark execution.
Changes:
- Add result dumping hooks to
benchmark/run_benchmark.pyvia--result-dirand aresult_dataframe()engine hook. - Introduce
benchmark/verify_results.pyto compare dumped CSV results vs committed answers and produce a job-summary table with CI gating. - Wire CI to upload per-query result CSV artifacts and run a downstream
verify-correctnessjob in.github/workflows/benchmark.yml.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
benchmark/verify_results.py |
New engine-free verifier that compares dumped results to committed answers and emits a markdown correctness report (intended to fail CI on mismatches). |
benchmark/run_benchmark.py |
Adds normalized result dumping in isolated subprocesses and CLI flags to gate dumping on presence of committed answers. |
benchmark/answers/README.md |
Updates documentation to describe the new downstream verification approach and semantics. |
.github/workflows/benchmark.yml |
Uploads dumped result CSVs from benchmark jobs and adds a verify-correctness downstream job to reuse artifacts and gate CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = answer.columns[i] | ||
| a = answer.iloc[:n, i].reset_index(drop=True) | ||
| b = result.iloc[:n, i].reset_index(drop=True) | ||
| if pd.api.types.is_float_dtype(a) or pd.api.types.is_float_dtype(b): |
Comment on lines
+97
to
+102
| def boundary_only(issues: list[str], answer: pd.DataFrame) -> bool: | ||
| """True if every discrepancy is confined to the final row (LIMIT-boundary tie).""" | ||
| if not issues: | ||
| return False | ||
| last = len(answer) - 1 | ||
| return all(f"row {last}:" in msg or f"at row {last}" in msg for msg in issues) |
Comment on lines
+134
to
+141
| if not result_csv.exists(): | ||
| if status == "timeout": | ||
| return "timeout", None | ||
| if status == "not_started": | ||
| return "oom", None | ||
| if status == "error": | ||
| return "run_error", None | ||
| return "no_result", None |
jiayuasu
force-pushed
the
feature/correctness-harness
branch
from
July 23, 2026 08:40
ac2d76f to
1559eb5
Compare
…esults
Adds a correctness check that reuses the benchmark run instead of re-executing
queries in a second workflow: the benchmark jobs already run every
(engine, query, scale factor), so they now also dump each result, and a
downstream verify job compares those dumps to the committed ground-truth answers
(benchmark/answers/sf<sf>). A mismatch — or a result that reported success but is
missing — fails CI.
run_benchmark.py:
- New --result-dir: reuse the timed run's result (no extra execution) to write
<engine>_<query>_result.csv. DuckDB captures column names during the timed run
so its fetchall result becomes a DataFrame without re-running; other engines
reuse the DataFrame they already return. Normalizes durations -> seconds,
decimals/object-decimals -> float, timestamps -> datetime. Writes csv only, so
no pyarrow filesystem is touched inside a SedonaDB worker; pandas/numpy stay out
of the module top to preserve the import-order isolation. Gated on committed
answers existing for the scale factor, so SF10 (no answers yet) does no extra
work.
benchmark/verify_results.py (new, engine-free):
- Derives comparison semantics from the canonical answer schema (the type-faithful
q<n>.parquet): integer keys/counts, timestamps and strings match exactly, and
only floating-point metrics use rtol/atol.
- The LIMIT-boundary-tie exception is restricted to eligible capped LIMIT queries
and only tolerates a final-row identity swap where every float ordering metric
still ties within tolerance — never a single-row/non-LIMIT query or a numeric
difference.
- Result-presence semantics: reported-success-but-no-dump fails (a silent dump
failure); explicit timeout/error/OOM is tolerated; a framework that produced no
record at all (its whole benchmark job failed, e.g. an install error) is shown
as "no result" but does not fail the gate — it left no answer that could be
wrong, and its own red job surfaces it.
- Robust by construction: a result that cannot be read/compared becomes a warning
cell, never a crash, so one broken framework can't take down the summary.
- Renders a per-engine correctness table to the job summary and exits non-zero on
any real failure.
benchmark.yml:
- Engine jobs pass --result-dir and upload the result csv alongside the timing
json; pandas added to the Spatial Polars / PyCanopy jobs.
- New verify-correctness job: needs the benchmark jobs (runs even if some failed),
installs pandas + pyarrow, runs verify_results.py, always publishes the table to
the job summary (with a fallback note if none was produced), and fails CI on a
mismatch.
Part of #126.
jiayuasu
force-pushed
the
feature/correctness-harness
branch
from
July 23, 2026 20:55
1559eb5 to
a238262
Compare
The correctness verification (verify_results.py) surfaced five (engine, query) results that diverged from the SedonaDB/DuckDB ground-truth answers. All are real implementation bugs, not tolerance issues: Q5 was missing a whole column. GeoPandas and PyCanopy computed the repeat-customer trip count for the `> 5` filter but dropped it from the output; the reference (and the answer) return it as `dropoff_count`. Add it to the projection. Spatial Polars Q7 used the wrong metre/degree constant. It multiplied the straight-line degree distance by 111111, while the reference divides by 0.000009 (= x111111.111...); the ~1e-6 gap tripped the tolerance. Use /0.000009 to match the reference and the other engines. PyCanopy Q6/Q10 lost precision on avg_distance. t_distance is decimal(15,5), and a decimal mean stays at scale 5 and rounds the result (e.g. 0.00086 vs 0.000865); SedonaDB and the other engines average in float. Cast t_distance to float before averaging. Verified at SF1 against the committed answers: GeoPandas Q5, Spatial Polars Q7 and PyCanopy Q10 now match. PyCanopy Q5/Q6 use the same fixes (column projection and the identical float cast) but need PyCanopy 0.3.3 (Linux-only wheel) to run, so they are verified by CI rather than locally. Part of #126.
Four fixes so the correctness gate cannot pass without genuinely verifying results: - Fail when nothing was verified. A failed artifact download now fails the verify job (the download step no longer swallows errors), and verify_results.py fails if zero (engine, query) pairs were actually compared — so a missing/empty results set can't pass silently. A single framework that produced no result is still reported (❔) without failing, so the summary and the gate both survive one broken engine. - Treat an unreadable result as a failure. A result file that exists but cannot be read/compared is now a failing verdict (was reported but non-failing); the summary still renders (crash-safe per cell). run_benchmark.py writes each dump to a temp file and atomically replaces the destination, so a process killed mid-write never leaves a partial csv. - Remove Q8 from the LIMIT-boundary-tie tolerance. Q8 orders by an integer count with an integer-key tiebreaker, so its ordering is deterministic and must match exactly; the boundary exception only guards float ordering ties and would otherwise accept an arbitrary final-row count. - Keep result serialization outside the query timeout. The worker queues its timing result before serializing; if the process is still alive at the timeout, the runner checks the queue and, if the query already finished, gives serialization a bounded grace instead of charging it to the query timeout. Part of #126.
jiayuasu
marked this pull request as ready for review
July 24, 2026 07:47
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.
Summary
Second of two PRs for #126. #127 committed the ground-truth answers; this PR adds the correctness check that verifies every participating engine returns them, and fails CI on a mismatch.
Rather than a separate workflow that re-executes all 60 (engine × query) runs a second time, it reuses the benchmark's runs. The benchmark already runs every
(engine, query, scale factor), so it now also dumps each result, and a downstreamverify-correctnessjob — whichneedsthe benchmark jobs — compares those dumps to the committed answers in a clean, engine-free step. No duplicated execution; the comparison happens where no engine is installed, so none of the SedonaDB/pyarrow import-order pitfalls apply.What changed
run_benchmark.py— new--result-dir. After a query's timed runs, it captures the result (newresult_dataframe()hook), normalizes it (durations → seconds, decimals → float, timestamps → datetime), and writes<engine>_<query>_result.csv. Runs in the same isolated-subprocess pattern and writes csv only (never parquet), so no pyarrow filesystem is touched inside a SedonaDB worker; pandas/numpy stay out of the module top to preserve the import-order isolation. Gated on committed answers existing for the scale factor, so SF10 (no answers yet) does no extra work.benchmark/verify_results.py(new, engine-free) — compares each dumped result to its committed answer csv and renders a per-engine correctness table to the job summary:avg_durationvsavg_duration_seconds).rtol=1e-6,atol=1e-9; a within-toleranceLIMIT-boundary-row difference passes.benchmark.yml— engine jobs pass--result-dirand upload the result csv alongside the timing json (pandas added to the Spatial Polars / PyCanopy jobs, whose dumps convert polars → pandas). Newverify-correctnessjob:needsthe benchmark jobs, downloads their results, runsverify_results.py, publishes the correctness table to the job summary, and fails on a mismatch.Example summary (published to the job)
✅ matches · ❌ mismatch (fails CI) · ⏱️ couldn't compute (not a failure) · — not verified.
Local validation (SF1)
verify_results.pyexits 1 (CI would fail).Spatial Polars and PyCanopy use the identical dump path and are exercised by CI.
Notes