petern48 commented on code in PR #234:
URL: https://github.com/apache/sedona-db/pull/234#discussion_r2452446890
##########
python/sedonadb/python/sedonadb/testing.py:
##########
@@ -269,11 +269,74 @@ def assert_result(self, result, expected, **kwargs) ->
"DBEngine":
result_pandas = self.result_to_pandas(result)
pandas.testing.assert_frame_equal(result_pandas, expected,
**kwargs)
elif isinstance(expected, list):
- result_tuples = self.result_to_tuples(result, **kwargs)
- if result_tuples != expected:
- raise AssertionError(
- f"Expected:\n {expected}\nGot:\n {result_tuples}"
- )
+ result_table = self.result_to_table(result)
+ geom_cols_schema = _geometry_columns(result_table.schema)
+
+ if geom_cols_schema:
+ import shapely
Review Comment:
Hmm, this logic is much more complicated than I had in mind. I assume it's
all because tests were failing otherwise. I actually had a simpler short-term
fix in mind. Is there a reason you didn't go with a simpler Boolean flag
approach, that we only turn on when needed?
```python
def assert_result(sef result, expected, **kwargs):
...
if kwargs["geom_compare"]:
< the ~5 line shapely comparison code I had above >
else:
< old existing str comparison logic >
# then we add the new `geom_compare=True` flag only in test_st_unaryunion()
```
It might not support every case, but I think it should be enough for this
PR. Future cases like CRS can be handled later (if they're needed). The reason
I imagined a simpler boolean flag approach was to keep this change isolated
(though something we could easily turn on/off) and keep the same testing logic
for the rest of the functions. The reason was that I wonder if there's any
reason we specifically would want to keep the string comparison approach for
some functions (ie. returning False for different orderings). I can't think of
any at the moment, though. From a reviewer's perspective, modifying this for
all cases means we need to scrutinize your PR deeper since you're no longer
just adding new functionality. Even if it passes, we need to make sure you're
not doing anything negative to the existing code.
WDYT? If you feel strongly about this approach, let me know, and I will look
to deeper understand it later. Also, feel free to move the shapely import to
the top. I only put it inline in my example for conciseness.
--
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]
