Copilot commented on code in PR #128: URL: https://github.com/apache/sedona-spatialbench/pull/128#discussion_r3636313138
########## benchmark/verify_results.py: ########## @@ -0,0 +1,277 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Verify SpatialBench correctness by comparing the benchmark run's result dumps +against the committed ground-truth answers, and render a summary table. + +This runs as a downstream CI job that *needs* the benchmark jobs and reuses their +output: the benchmark run writes each query's normalized result to +``<engine>_<query>_result.csv`` (see run_benchmark.py --result-dir); this script +reads those dumps plus the committed answers in ``benchmark/answers/sf<sf>`` and +compares them. It is engine-free (only pandas/numpy), so none of the SedonaDB / +pyarrow import-order pitfalls apply here. + +Comparison rules (same as the answers were blessed with): + * columns compared by position (engines name e.g. avg_duration differently) + * integer keys / strings / timestamps: exact + * floats (distances, areas, IoU, seconds): rtol=1e-6, atol=1e-9 + * a within-tolerance difference confined to the final LIMIT-boundary row passes + +Exit status: non-zero if any engine's result MISMATCHES its committed answer, so a +regression fails CI. An engine that could not produce a result (timeout / error / +OOM, e.g. DuckDB's lateral-join Q12 at scale) is reported but does not fail the +job — that is a runtime issue surfaced by the benchmark summary, not a wrong answer. +""" + +import argparse +import json +import sys +from datetime import datetime, timezone +from pathlib import Path + +import numpy as np +import pandas as pd + +QUERY_COUNT = 12 +ENGINES = ["duckdb", "geopandas", "sedonadb", "spatial_polars", "pycanopy"] +ENGINE_ICONS = { + "sedonadb": "🌵 SedonaDB", + "duckdb": "🦆 DuckDB", + "geopandas": "🐼 GeoPandas", + "spatial_polars": "🐻❄️ Spatial Polars", + "pycanopy": "🌴 PyCanopy", +} + + +def compare(answer: pd.DataFrame, result: pd.DataFrame, rtol: float, atol: float) -> list[str]: + """Positional comparison of a result against the answer.""" + if answer.shape[1] != result.shape[1]: + return [f"column count differs: answer {list(answer.columns)} vs engine {list(result.columns)}"] + issues = [] + if len(answer) != len(result): + issues.append(f"row count differs: answer {len(answer)} vs engine {len(result)}") + n = min(len(answer), len(result)) + for i in range(answer.shape[1]): + 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): + va = pd.to_numeric(a, errors="coerce").to_numpy(dtype=float) + vb = pd.to_numeric(b, errors="coerce").to_numpy(dtype=float) + close = np.isclose(va, vb, rtol=rtol, atol=atol, equal_nan=True) + bad = int((~close).sum()) + if bad: + j = int(np.where(~close)[0][0]) + issues.append( + f"col {i} ('{name}'): {bad}/{n} float mismatch " + f"(first at row {j}: {va[j]!r} vs {vb[j]!r})" + ) + else: + neq = a.astype("string").fillna("<NA>") != b.astype("string").fillna("<NA>") + bad = int(neq.sum()) + if bad: + j = int(np.where(neq.to_numpy())[0][0]) + issues.append( + f"col {i} ('{name}'): {bad}/{n} mismatch " + f"(first at row {j}: {a.iloc[j]!r} vs {b.iloc[j]!r})" + ) + return issues + + +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) Review Comment: `boundary_only()` currently treats any discrepancy confined to the last row as tolerable, including out-of-tolerance float mismatches. That can incorrectly pass real numeric regressions that happen to affect only the final row. The boundary-tie exception should never override a `float mismatch`. ########## benchmark/verify_results.py: ########## @@ -0,0 +1,277 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Verify SpatialBench correctness by comparing the benchmark run's result dumps +against the committed ground-truth answers, and render a summary table. + +This runs as a downstream CI job that *needs* the benchmark jobs and reuses their +output: the benchmark run writes each query's normalized result to +``<engine>_<query>_result.csv`` (see run_benchmark.py --result-dir); this script +reads those dumps plus the committed answers in ``benchmark/answers/sf<sf>`` and +compares them. It is engine-free (only pandas/numpy), so none of the SedonaDB / +pyarrow import-order pitfalls apply here. + +Comparison rules (same as the answers were blessed with): + * columns compared by position (engines name e.g. avg_duration differently) + * integer keys / strings / timestamps: exact + * floats (distances, areas, IoU, seconds): rtol=1e-6, atol=1e-9 + * a within-tolerance difference confined to the final LIMIT-boundary row passes + +Exit status: non-zero if any engine's result MISMATCHES its committed answer, so a +regression fails CI. An engine that could not produce a result (timeout / error / +OOM, e.g. DuckDB's lateral-join Q12 at scale) is reported but does not fail the +job — that is a runtime issue surfaced by the benchmark summary, not a wrong answer. +""" + +import argparse +import json +import sys +from datetime import datetime, timezone +from pathlib import Path + +import numpy as np +import pandas as pd + +QUERY_COUNT = 12 +ENGINES = ["duckdb", "geopandas", "sedonadb", "spatial_polars", "pycanopy"] +ENGINE_ICONS = { + "sedonadb": "🌵 SedonaDB", + "duckdb": "🦆 DuckDB", + "geopandas": "🐼 GeoPandas", + "spatial_polars": "🐻❄️ Spatial Polars", + "pycanopy": "🌴 PyCanopy", +} + + +def compare(answer: pd.DataFrame, result: pd.DataFrame, rtol: float, atol: float) -> list[str]: + """Positional comparison of a result against the answer.""" + if answer.shape[1] != result.shape[1]: + return [f"column count differs: answer {list(answer.columns)} vs engine {list(result.columns)}"] + issues = [] + if len(answer) != len(result): + issues.append(f"row count differs: answer {len(answer)} vs engine {len(result)}") + n = min(len(answer), len(result)) + for i in range(answer.shape[1]): + 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): Review Comment: `compare()` decides whether to use tolerance based on *either* side being float. If an engine accidentally serializes an integer key column as float, the comparison will switch to `np.isclose()` and apply rtol/atol to a column that is supposed to be exact, potentially masking integer-key regressions. Use the answer column's dtype (canonical fixture) to decide float-vs-exact comparison. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
