paleolimbot commented on code in PR #1043:
URL: https://github.com/apache/sedona-db/pull/1043#discussion_r3583125357
##########
python/sedonadb/tests/functions/test_aggregate.py:
##########
@@ -168,6 +168,32 @@ def test_st_envelope_nontrivial_input(eng, geoarrow_data):
geopandas.testing.assert_geodataframe_equal(df, expected, check_crs=False)
[email protected]("eng", [SedonaDB])
+def test_st_convexhull_agg_matches_collect_chain(eng, con):
Review Comment:
I think there are two more things we should test here:
- Parameterize this to check for a few numbers of groups including no groups
(i.e., unlikely to go through the GroupsAccumulator)
- Add a test that aggregates zero rows
- Optional, but a test that confirms behaviour of a few known corner cases
would help keep future PRs doing optimizations or refactoring honest. I think
you can parameterize the like `(["POINT (0 1)", "POINT (0 1)"], "LINESTRING (0
0 1 1)")` and use `geoarrow.pyarrow.as_wkb([...values...])` + `pyarrow.table()`
to generate the values (and `f"ST_ConvexHull(GEOMETRYCOLLECTION({',
'.join(values_with_nulls_as_empties)}))"` to generate the expected value).
##########
python/sedonadb/tests/functions/test_aggregate.py:
##########
@@ -168,6 +168,32 @@ def test_st_envelope_nontrivial_input(eng, geoarrow_data):
geopandas.testing.assert_geodataframe_equal(df, expected, check_crs=False)
[email protected]("eng", [SedonaDB])
+def test_st_convexhull_agg_matches_collect_chain(eng, con):
+ eng = eng.create_or_skip()
+ num_groups = 100
+
+ df_points = con.sql("""
+ SELECT id, geometry FROM sd_random_geometry(
+ '{"geom_type": "Point", "num_rows": 2000, "seed": 9728}'
+ )
+ """)
+ eng.create_table_arrow("df_points", df_points.to_arrow_table())
+
+ eng.assert_query_result(
+ f"""
+ SELECT bool_and(ST_Equals(hull_new, hull_old)) FROM (
+ SELECT
+ ST_ConvexHull_Agg(geometry) AS hull_new,
+ ST_ConvexHull(ST_Collect_Agg(geometry)) AS hull_old
+ FROM df_points
+ GROUP BY id % {num_groups}
+ )
+ """,
+ True,
+ )
Review Comment:
Optional, but this will generate more informative errors when it fails if
you collect the first version to pandas, collect the second version to pandas,
and use `geopandas.testing.assert_geodataframe_equal()`.
--
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]
