Repository navigation
Spell the grouped convex hull as ST_Collect_Agg and align the documented queries - #142
Merged
jiayuasu merged 2 commits intoSep 1, 2026
Merged
Conversation
james-willis
force-pushed
the
fix-documented-q5-st-collect-agg
branch
from
August 31, 2026 22:35
77604f1 to
ca8c351
Compare
james-willis
marked this pull request as ready for review
August 31, 2026 22:37
james-willis
requested review from
jiayuasu and
prantogg
and removed request for
prantogg
August 31, 2026 22:37
prantogg
approved these changes
Aug 31, 2026
james-willis
marked this pull request as draft
August 31, 2026 22:47
james-willis
force-pushed
the
fix-documented-q5-st-collect-agg
branch
from
August 31, 2026 22:47
ca8c351 to
029f65e
Compare
…ted queries
Q5 computes a convex hull over each customer-month's dropoff locations. The base class -- documented as the Sedona/Spark SQL dialect -- spelled it as a scalar over a materialized array, ST_Collect(ARRAY_AGG(...)), while SedonaDBSpatialBenchBenchmark overrode Q5 for the sole purpose of spelling it as the native aggregate ST_Collect_Agg. Sedona Spark ships that aggregate too, registered in spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_aggregates.scala as call_udf("ST_Collect_Agg", geometry), so the base dialect can say it directly and SedonaDB needs no override. ARRAY_AGG also builds a full array of geometries per group before ST_Collect runs, where a native aggregate accumulates into one collection incrementally.
DuckDB inherits the base Q5 and DuckDB spatial has no ST_Collect_Agg -- its ST_Collect is a scalar over a GEOMETRY[], and its geometry aggregates are ST_Union_Agg and ST_Envelope_Agg -- so the ARRAY_AGG spelling moves into a DuckDB override rather than disappearing. Each override now sits where the dialect genuinely differs: DuckDB has no ST_Collect_Agg, Databricks has neither and uses ST_Union_Agg, and SedonaDB runs the base suite unmodified. The Databricks comment is reworded to name the function the base now uses; its SQL is untouched.
With the override gone, SedonaDBSpatialBenchBenchmark had no queries left and is deleted. Its docstring claimed it existed to name the dialect, but dialect() is never called anywhere in the repo -- every mention is a definition, a docstring, or an unrelated local variable, and the harness gets its engine name from a string literal at run_benchmark.py:393 and its display label from separate maps. The class is therefore repointed to the base in the two places that referenced it, print_queries.py's CLI map (keeping the "SedonaDB" key, so `print_queries.py SedonaDB` still works) and run_benchmark.py's dialect map. This widens the diff into run_benchmark.py, which is otherwise untouched; the class is only deletable because the base now carries the aggregate spelling, so it is a direct consequence of this change rather than an unrelated cleanup. DuckDB and Databricks keep their classes because they still override real queries -- DuckDB q5 and q12, Databricks q5, q7 and q12 -- so only the empty one goes. Verified mechanically: the query dict get_sql_queries("sedonadb") returns is byte-identical before and after, same sha256 over all twelve queries.
dialect() is dead code on the remaining classes too, but that predates this change and is left alone rather than widening scope further.
This requires Sedona >= 1.8.1 for the base dialect. Measured, one Spark 3.5.3 session per version: 1.7.2 and 1.8.0 reject ST_Collect_Agg with UNRESOLVED_ROUTINE, 1.8.1 and 1.9.1 accept it. Nothing in the repo pins a Sedona version, docs/requirements.txt is an unpinned apache-sedona[db] that resolves to 1.9.1, and there is no Sedona Spark runner in the benchmark harness, so no engine the harness runs is affected. DuckDB in particular is unaffected because it now carries its own override.
The documented queries had drifted from the executable ones in two ways, both fixed here. Q5 could not run at all: the Queries page and notebooks/queries.ipynb spelled it ST_Collect(ST_GeomFromWKB(...)) inside an sd.sql(...) call, and SedonaDB has no ST_Collect, so it failed at planning time with "Invalid function 'st_collect'". Q7 ran but multiplied by 111111 where every executable query divides by 0.000009; since 1/0.000009 is 111111.111..., the documented Q7 was off by exactly 1e-6 relative -- not inside the CI tolerance of rtol=1e-6 but exactly on it, its detour_ratio reaching 1.000001e-06 and passing only because numpy's isclose adds an atol term.
The divide form is the suite-wide convention and 111111 appeared only in those three doc lines. It is used by the executable SQL (print_queries.py:165,328), by every other engine implementation (geopandas_queries.py:306, spatial_polars.py:331-333, pycanopy_queries.py:260), and by wherobots/benchmark-dashboard. The repo documents it as the convention in docs/geography-queries.md:37 and its table row "| Unit conversion in SQL | / 0.000009 | none needed |", and it is structural: the radii 0.45 (50 km), 0.045 (5 km) and 0.0045 (500 m) are all exact multiples of 0.000009.
Verified. The executable SQL each engine runs is unchanged except the base dialect's Q5: comparing every query before and after with comments and whitespace normalized away, SedonaDB, DuckDB and Databricks are all unchanged, and DuckDB's Q5 is byte-identical to what it ran before. On real SF1 data all three spellings -- the new base ST_Collect_Agg, the old ARRAY_AGG, and the Databricks ST_Union_Agg form run through Sedona Spark, which also has that function -- return bitwise identical results and match benchmark/answers/sf1/q5.csv, as do SedonaDB 0.4.1 and DuckDB 1.5.5 running their own dialects. All twelve documented queries were then executed against SedonaDB 0.4.1 at SF1 over all six tables, through both the documented `import sedona.db` and the standalone sedonadb package; all twelve run and all twelve match the committed answers under rtol=1e-6 with no atol slack, which Q7 previously did not. Q7 is now bit-for-bit identical to answers/sf1/q7.csv.
Stored notebook outputs were handled per cell. Q5's is left exactly as it was, because re-executing the corrected cell reproduces it byte for byte -- the committed output was already the right data attached to code that could not produce it. Q7's genuinely changed, since the constant moves the values in the sixth significant figure, so it is regenerated from a real execution of the corrected cell; the new values match answers/sf1/q7.csv exactly where the old ones did not.
Only the aggregate spelling and the metre constant change. SELECT lists, GROUP BY, HAVING, ORDER BY and LIMIT are untouched, the Chinese page gets the identical SQL edits with its translated comments left alone except the one stating the conversion, and benchmark/answers/ is not modified.
Signed-off-by: James Willis <james@wherobots.com>
james-willis
force-pushed
the
fix-documented-q5-st-collect-agg
branch
from
August 31, 2026 23:01
029f65e to
cd888c6
Compare
james-willis
marked this pull request as ready for review
September 1, 2026 00:02
jiayuasu
reviewed
Sep 1, 2026
| ST_GeomFromWKB(t.t_dropoffloc) | ||
| ) | ||
| ) * 111111 AS line_distance_m -- Approx. meters per degree | ||
| ) / 0.000009 AS line_distance_m -- 1 meter = 0.000009 degree |
Member
There was a problem hiding this comment.
Could we refresh the sample output below, and the matching block in queries.zh.md, to go with this conversion? The notebook now shows 11111.126052681648 / 8.99998789734414e-9 for the first row, while both rendered pages still show the old * 111111 values.
The SQL was aligned to divide by 0.000009 but the rendered tables still carried values computed under * 111111 (off by the 1.000001 ratio between the two constants). Copied verbatim from the re-executed notebook cell.
jiayuasu
approved these changes
Sep 1, 2026
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.
Q5 computes a convex hull over each customer-month's dropoff locations. This changes how that aggregation is spelled in the base dialect, and fixes two ways the documented queries had drifted from the executable ones.
1. The base dialect spells the hull as a scalar over an array
The base class is documented as the Sedona/Spark SQL dialect, but spelled Q5 as
ST_Collect(ARRAY_AGG(...)), whileSedonaDBSpatialBenchBenchmarkoverrode Q5 solely to spell it as the native aggregateST_Collect_Agg. Sedona Spark ships that aggregate too — registered inst_aggregates.scalaascall_udf("ST_Collect_Agg", geometry)— so the base can say it directly and SedonaDB needs no override.ARRAY_AGGalso builds a full array of geometries per group beforeST_Collectruns, where a native aggregate accumulates incrementally.DuckDB inherits the base Q5 and has no
ST_Collect_Agg, so theARRAY_AGGspelling moves into a DuckDB override rather than disappearing:SpatialBenchBenchmark(base)ST_Collect_AggDuckDBSpatialBenchBenchmarkST_Collect(ARRAY_AGG(…))SedonaDBSpatialBenchBenchmarkDatabricksSpatialBenchBenchmarkST_Union_AggSedonaDBSpatialBenchBenchmarkis deletedWith the override gone the class had no queries left. Its docstring would have claimed it existed to name the dialect, but
dialect()is never called anywhere in the repo — every mention is a definition, a docstring, or an unrelated local variable. The harness takes its engine name from a string literal atrun_benchmark.py:393and its display label from separate maps insummarize_results.pyandverify_results.py.So it is repointed to the base in the two places that referenced it:
print_queries.py's CLI map (keeping the"SedonaDB"key, soprint_queries.py SedonaDBstill works) andrun_benchmark.py's dialect map. This widens the diff intobenchmark/run_benchmark.py, which this PR does not otherwise touch — worth flagging, though the class is only deletable because the base now carries the aggregate spelling, so it is a direct consequence of the main change rather than an unrelated cleanup.DuckDB and Databricks keep their classes, because they still override real queries (DuckDB q5 + q12; Databricks q5, q7, q12). Only the empty one goes, so this is not an arbitrary asymmetry.
Verified mechanically rather than assumed: the query dict
get_sql_queries("sedonadb")returns is byte-identical before and after — same sha256 across all twelve queries.dialect()is dead code on the remaining classes too. That predates this PR and is left alone rather than widening scope again; possible follow-up if maintainers want it.This requires Sedona >= 1.8.1 for the base dialect
Measured, one Spark 3.5.3 session per version:
ST_Collect(ARRAY_AGG(…))ST_Collect_Agg(…)UNRESOLVED_ROUTINEUNRESOLVED_ROUTINEStating that plainly because it is a real floor. In this repo nothing pins a Sedona version,
docs/requirements.txtis an unpinnedapache-sedona[db]that resolves to 1.9.1, and there is no Sedona Spark runner inrun_benchmark.py— so no engine the harness runs is affected. DuckDB in particular is unaffected, because it now carries its own override.2. The documented Q5 could not run at all
The Queries page and
notebooks/queries.ipynbspelled itST_Collect(ST_GeomFromWKB(...))inside ansd.sql(...)call. SedonaDB has noST_Collect, so the documented query failed at planning time withInvalid function 'st_collect'. The geography suite's Q5 already usesST_Collect_Agg; this brings the geometry pages in line.3. The documented Q7 used a different metre constant
It multiplied by
111111where every executable query divides by0.000009. Those are not the same number —1/0.000009 = 111111.111…— so the documented Q7 was off by exactly 1e-6 relative, which is not inside the CI tolerance ofrtol=1e-6but exactly on it:detour_ratioreached 1.000001e-06 and passed only becausenumpy.iscloseadds anatolterm.111111appeared only in those three doc lines./ 0.000009is the convention in the executable SQL (print_queries.py:165,328), every other engine (geopandas_queries.py:306,spatial_polars.py:331-333,pycanopy_queries.py:260) andwherobots/benchmark-dashboard; the repo documents it indocs/geography-queries.md:37and its table row| Unit conversion in SQL | / 0.000009 | none needed |; and it is structural — the radii0.45(50 km),0.045(5 km) and0.0045(500 m) are all exact multiples of0.000009.Verification
No engine's executable SQL changes except the base dialect's. Comparing every query before and after with comments and whitespace normalized away:
The SedonaDB and DuckDB rows are taken through the harness's own
get_sql_queries()entry point, so they exercise the repointed maps rather than the classes directly.Every Q5 spelling agrees, by value, on real SF1 data — all bitwise identical to each other and matching
benchmark/answers/sf1/q5.csv:ST_Collect_Agg— Sedona Spark 1.9.1ARRAY_AGG— Sedona Spark 1.9.1ST_Union_Aggform — via Sedona Spark, which has that functionAll twelve documented queries were executed against SedonaDB 0.4.1 at SF1 over all six tables, through both the documented
import sedona.dband the standalonesedonadbpackage. Before, eleven ran and Q5 was the only failure; now all twelve run, and all twelve match the committed answers underrtol=1e-6with noatolslack — which Q7 previously did not.Q7 against
answers/sf1/q7.csv:* 111111)/ 0.000009)line_distance_mdetour_ratioIt is now bit-for-bit identical to the committed answer, and the documented Q7 is character-identical to the executable one.
Notebook outputs
Handled per cell. Q5's is left exactly as it was — re-executing the corrected cell reproduces it byte for byte, so the committed output was already the right data attached to code that could not produce it. Q7's is regenerated from a real execution, since the constant moves the values in the sixth significant figure; the new values match
answers/sf1/q7.csvexactly, the old ones did not. No other cell touched, and the notebook JSON round-trips with no formatting churn.benchmark/answers/is not modified. This supersedes #141, whose content is folded in here.CI status: the red checks are not from this change
The benchmark workflow shows failures, all of them GeoPandas or Spatial Polars at SF10. They are pre-existing infrastructure timeouts:
cancelledstep, not an assertion or an error — the SF10 runners time out.mainruns ofbenchmark.ymlfailed, and the failing set drifts between runs (three recent main runs failed 7, 10 and 12 jobs, with membership shifting), which is the signature of flaky infrastructure rather than a deterministic break.get_sql_queries()(run_benchmark.py:598,608); GeoPandas, Spatial Polars and PyCanopy each use{f"q{i}": None}and run their own DataFrame implementations.In fairness this run failed more SF10 jobs than the main run I compared against (17 vs 10), and seven of them were green in that particular baseline — but they are all in the same unstable GeoPandas/Spatial Polars SF10 set, and none of them touch the changed code path.
The checks that actually exercise this change are all green. DuckDB and SedonaDB are the only consumers of
print_queries.py, and every one of their jobs passes — including the two that matter most here:All 24 SedonaDB jobs and all 24 DuckDB jobs pass, at both scale factors.
🤖 Generated with Claude Code