Add row-level geo distance check and fix documentation examples - #1510
Add row-level geo distance check and fix documentation examples#1510simplegaurav wants to merge 3 commits into
Conversation
|
All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
Adds a row-level geo check that flags values farther than a maximum geodesic distance from a reference geography. Distance is measured in meters along the WGS 84 ellipsoid via `st_distance` on GEOGRAPHY values, so the check is meaningful for global data where planar GEOMETRY distances are not. The reference accepts a literal WKT/WKB value or a Column expression, and the maximum distance accepts a number, a Column, or a SQL expression so the radius can vary per row. Null column values and null distances are skipped; unparseable column and reference values are reported separately so the message names the value that has to be fixed. Numeric distance literals are validated up front: negative, NaN, infinite and boolean values are rejected with InvalidParameterError. The convert_column / convert_reference_geometry flags default to False, matching the existing is_geo_* relationship checks, and the rendering of the offending value follows that contract - the raw value when the input is converted from WKT/WKB, st_astext when the column is already a native GEOGRAPHY. Covered by unit tests, integration tests, the all-row-geo metadata fixture, the programmatic class-based integration test, a performance benchmark, and the quality checks reference documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The programmatic examples for is_geo_contains, is_geo_covers,
is_geo_intersects, is_geo_touches and is_geo_within construct the rules
with DQDatasetRule, but all of these checks are registered with
@register_rule("row"). Copying the snippets as written fails:
InvalidCheckError: Function 'is_geo_within' is not a dataset-level
rule. Use DQRowRule instead.
Switch the seven affected examples to DQRowRule and add the missing
import. are_polygons_mutually_disjoint is a genuine dataset-level rule
and is left unchanged.
These examples are not exercised by
test_apply_checks_all_geo_checks_using_classes, which is why the error
went unnoticed; backfilling that coverage needs a workspace to pick
reference geometries that pass, and is left as a follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f1ef258 to
56fbfdb
Compare
ghanse
left a comment
There was a problem hiding this comment.
Very good contribution. Left a few minor comments. Need to add 1 test case.
|
|
||
| Both the target column and the reference geometry are always handled as `GEOGRAPHY`. | ||
| When conversion is requested (*convert_column* or *convert_reference_geometry* set to True), | ||
| *try_to_geography* is applied to parse the value from any supported format (WKT, WKB, EWKT, EWKB). |
There was a problem hiding this comment.
We should include GeoJSON in the supported formats as well.
There was a problem hiding this comment.
I think we're missing a test case for convert_column=False
Include GeoJSON in the documented input formats. The docstring listed the try_to_geometry formats (WKT, WKB, EWKT, EWKB) copied from the sibling relationship checks, but this check parses with try_to_geography, which also accepts GeoJSON. Updated the docstring, the reference_geometry argument description and the reference documentation table. Add integration coverage for convert_column=False. Both new tests build a native GEOGRAPHY column with try_to_geography and then leave the convert flags at their defaults, covering the pass and violation paths and exercising the st_astext rendering branch used when the column is already a GEOGRAPHY value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review — all three addressed in GeoJSON in supported formats Good catch. The docstring listed the Missing Added two integration tests covering the native def test_is_geo_within_distance_native_geography_violation(skip_if_runtime_not_geo_compatible, spark):
"""A native GEOGRAPHY value outside the radius is flagged, with the value rendered via st_astext."""
point = "POINT(5.05 52.37)"
test_df = spark.createDataFrame([[point]], _GEO_SCHEMA).select(
F.call_function("try_to_geography", F.col("geom")).alias("geom")
)
condition = is_geo_within_distance("geom", F.call_function("try_to_geography", F.lit(_POINT_INSIDE)), 1000)
...They build the column with I assert on the condition column alone rather than also selecting One thing I noticed while writing those No test in the repo currently builds a native Thanks for opening #1513. |
ghanse
left a comment
There was a problem hiding this comment.
Left a few minor suggestions.
| text_value_col = col_expr.cast("string") if convert_column else F.call_function("st_astext", col_geog) | ||
|
|
||
| invalid_column_message = F.concat_ws( | ||
| "", | ||
| F.lit("value `"), | ||
| text_value_col, | ||
| F.lit(f"` in column `{col_expr_str}` is not a valid geography"), | ||
| ) | ||
| invalid_reference_message = F.lit(f"reference geometry for column `{col_expr_str}` is not a valid geography") | ||
| too_far_message = F.concat_ws( | ||
| "", | ||
| F.lit("value `"), | ||
| text_value_col, | ||
| F.lit(f"` in column `{col_expr_str}` is farther than "), | ||
| distance_expr.cast("string"), | ||
| F.lit(" meters from the reference geometry"), | ||
| ) |
There was a problem hiding this comment.
This may not leave the most useful messages if the user passes WKB/EWKB with convert_column=True. For other checks, we surround the parsed geometry with st_astext:
| text_value_col = col_expr.cast("string") if convert_column else F.call_function("st_astext", col_geog) | |
| invalid_column_message = F.concat_ws( | |
| "", | |
| F.lit("value `"), | |
| text_value_col, | |
| F.lit(f"` in column `{col_expr_str}` is not a valid geography"), | |
| ) | |
| invalid_reference_message = F.lit(f"reference geometry for column `{col_expr_str}` is not a valid geography") | |
| too_far_message = F.concat_ws( | |
| "", | |
| F.lit("value `"), | |
| text_value_col, | |
| F.lit(f"` in column `{col_expr_str}` is farther than "), | |
| distance_expr.cast("string"), | |
| F.lit(" meters from the reference geometry"), | |
| ) | |
| text_value_col = col_expr.cast("string") if convert_column else F.call_function("st_astext", col_geog) | |
| invalid_column_message = F.concat_ws( | |
| "", | |
| F.lit("value `"), | |
| text_value_col, | |
| F.lit(f"` in column `{col_expr_str}` is not a valid geography"), | |
| ) | |
| invalid_reference_message = F.lit(f"reference geometry for column `{col_expr_str}` is not a valid geography") | |
| too_far_value_col = F.call_function("st_astext", col_geog) | |
| too_far_message = F.concat_ws( | |
| "", | |
| F.lit("value `"), | |
| too_far_value_col, | |
| F.lit(f"` in column `{col_expr_str}` is farther than "), | |
| distance_expr.cast("string"), | |
| F.lit(" meters from the reference geometry"), | |
| ) |
| """ | ||
| # `bool` is a subclass of `int`, so it would otherwise slip through as a 0/1 metre radius. | ||
| if isinstance(distance, bool) or ( | ||
| isinstance(distance, (int, float)) and not (math.isfinite(distance) and distance >= 0) |
There was a problem hiding this comment.
math.isfinite may overflow on very large input distances. We might want to catch this and raise InvalidParameterError?
Changes
Adds
is_geo_within_distance, a row-level geospatial check that flags values farther than amaximum geodesic distance from a reference geography.
Distance is measured in meters along the WGS 84 ellipsoid via
st_distanceonGEOGRAPHYvalues, so the check is meaningful for global data where planar
GEOMETRYdistances are not.A row is reported when the shortest distance to the reference is strictly greater than
distance.Behaviour
reference_geometryaccepts a literal WKT/WKB/EWKT/EWKB string or bytes value, or aColumnexpression. A plain string is always a literal, never a column name.
distanceaccepts a non-negative number, aColumn, or a string SQL expression, so the radiuscan vary per row.
distanceevaluates to null are also skipped, sincethe comparison is unknown rather than violated.
so the error names the value that actually has to be fixed.
distanceliterals are validated up front: negative, NaN, infinite and boolean valuesraise
InvalidParameterError. (boolis a subclass ofint, so it is rejected explicitlyrather than silently becoming a 0/1 metre radius.)
Consistency with existing geo checks
convert_column/convert_reference_geometrydefault toFalse, matching every existingis_geo_*relationship check, so the two are not surprising to switch between.<column>_is_not_within_distance_from_reference_geometry, following the_reference_geometrysuffix family. Including the reference in the alias also avoids acollision when the same column is checked against two different radii.
converted from WKT/WKB (where
st_astextwould be NULL on exactly the unparseable values themessage is about), and
st_astextwhen the column is already a nativeGEOGRAPHY.Second commit — pre-existing documentation bug
The programmatic examples for seven
is_geo_*checks (is_geo_contains,is_geo_covers×2,is_geo_intersects×2,is_geo_touches,is_geo_within) construct rules withDQDatasetRule,but all of these are registered with
@register_rule("row"). Copying the snippets as writtenfails:
Fixed to
DQRowRuleplus the missing import.are_polygons_mutually_disjointis a genuinedataset-level rule and is left unchanged. These examples are not covered by
test_apply_checks_all_geo_checks_using_classes, which is why it went unnoticed.Tests
Unit (
tests/unit/test_geo_check_funcs.py) — literal andColumnreferences, WKB reference,Columnand SQL-expression distances, zero distance, parametrized rejection of negative / NaN /±inf / boolean distances, and alias assertions for both the converted and native-
GEOGRAPHYpaths.Registered in
EXPECTED_PARAMETER_ORDERintests/unit/test_check_func_signatures.py.Integration (
tests/integration/test_row_checks_geo.py) — inside radius, on the reference atzero radius, outside radius, per-row radius via a column, null radius skipped, and the invalid
column and invalid reference paths asserted separately. Added to
tests/resources/all_row_geo_checks.yamland totest_apply_checks_all_geo_checks_using_classes.Performance (
tests/perf/test_apply_checks.py) —test_benchmark_is_geo_within_distance,alongside the existing
is_geo_coversbenchmarks.Documentation and Demos
docs/dqx/docs/reference/quality_checks.mdx— added the row-level checks table entry, a YAMLusage example, and a programmatic
DQRowRuleexample, each placed at the end of the geo group.Also contains the
DQDatasetRule→DQRowRulefix described above.