Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for geography columns #1627

Merged
merged 1 commit into from
Dec 22, 2024
Merged

Add support for geography columns #1627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions martin/src/pg/query_tables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ SELECT
FROM (
SELECT
ST_AsMVTGeom(
ST_Transform(ST_CurveToLine({geometry_column}), 3857),
ST_Transform(ST_CurveToLine({geometry_column}::geometry), 3857),
ST_TileEnvelope($1::integer, $2::integer, $3::integer),
{extent}, {buffer}, {clip_geom}
) AS geom
Expand Down Expand Up @@ -223,11 +223,11 @@ async fn calc_bounds(
.await?
.query_one(&format!(
r#"
WITH real_bounds AS (SELECT ST_SetSRID(ST_Extent({geometry_column}), {srid}) AS rb FROM {schema}.{table})
WITH real_bounds AS (SELECT ST_SetSRID(ST_Extent({geometry_column}::geometry), {srid}) AS rb FROM {schema}.{table})
SELECT ST_Transform(
CASE
WHEN (SELECT ST_GeometryType(rb) FROM real_bounds LIMIT 1) = 'ST_Point'
THEN ST_SetSRID(ST_Extent(ST_Expand({geometry_column}, 1)), {srid})
THEN ST_SetSRID(ST_Extent(ST_Expand({geometry_column}::geometry, 1)), {srid})
ELSE (SELECT * FROM real_bounds)
END,
4326
Expand Down
43 changes: 38 additions & 5 deletions martin/src/pg/scripts/query_available_tables.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ WITH
JOIN pg_opclass op ON
op.oid = ix.indclass[0] AND
op.opcname IN ('gist_geometry_ops_2d', 'spgist_geometry_ops_2d',
'brin_geometry_inclusion_ops_2d')
'brin_geometry_inclusion_ops_2d',
'gist_geography_ops')
GROUP BY 1, 2, 3),
--
annotated_geometry_columns AS (
Expand All @@ -38,6 +39,7 @@ WITH
f_geometry_column AS geom,
srid,
type,
-- 'geometry' AS column_type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how we could distinguish between tables using a geometry vs geography column.
I initially thought we might need it, but the approach of simply casting everything to ::geometry seems to be working well, so this can be removed unless we want to expose this information to the code for some other reason.

COALESCE(class.relkind = 'v', false) AS is_view,
bool_or(sic.column_name is not null) as geom_idx
FROM geometry_columns
Expand All @@ -50,6 +52,33 @@ WITH
geometry_columns.f_table_name = sic.table_name AND
geometry_columns.f_geometry_column = sic.column_name
GROUP BY 1, 2, 3, 4, 5, 6),
--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is geography metadata here substantially different from geometry to require a separate section? not a biggie, and not a blocker, but if possible - we should have just one consolidated sub-table here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few substantials differences here: geography columns are defined in the geography_columns table, and the name of the column containing the name of the spatial column is f_geography_column. I agree that the duplication here is not ideal, but I couldn't think of a better alternative to cover both use cases.

Perhaps one thing we could do is move the JOINs to the UNION'd table, at the cost of adding yet another "with query" block? Not sure which of the two would be more preferable.

annotated_geography_columns AS (
-- list of geography columns with additional metadata
SELECT f_table_schema AS schema,
f_table_name AS name,
f_geography_column AS geom,
srid,
type,
-- 'geography' AS column_type
COALESCE(class.relkind = 'v', false) AS is_view,
bool_or(sic.column_name is not null) as geom_idx
FROM geography_columns
JOIN pg_catalog.pg_class AS class
ON class.relname = geography_columns.f_table_name
JOIN pg_catalog.pg_namespace AS ns
ON ns.nspname = geography_columns.f_table_schema
LEFT JOIN spatially_indexed_columns AS sic ON
geography_columns.f_table_schema = sic.table_schema AND
geography_columns.f_table_name = sic.table_name AND
geography_columns.f_geography_column = sic.column_name
GROUP BY 1, 2, 3, 4, 5, 6),
--
annotated_geo_columns AS (
SELECT * FROM annotated_geometry_columns
UNION SELECT * FROM annotated_geography_columns
),
--
descriptions AS (
-- comments on table/views
SELECT
Expand All @@ -69,12 +98,16 @@ SELECT schema,
is_view,
geom_idx,
COALESCE(
jsonb_object_agg(columns.column_name, columns.type_name)
FILTER (WHERE columns.column_name IS NOT NULL AND columns.type_name != 'geometry'),
'{}'::jsonb
jsonb_object_agg(columns.column_name, columns.type_name)
FILTER (
WHERE columns.column_name IS NOT NULL
AND columns.type_name != 'geometry'
AND columns.type_name != 'geography'
),
'{}'::jsonb
) as properties,
dc.description
FROM annotated_geometry_columns AS gc
FROM annotated_geo_columns AS gc
LEFT JOIN columns ON
gc.schema = columns.table_schema AND
gc.name = columns.table_name AND
Expand Down
2 changes: 2 additions & 0 deletions martin/tests/pg_server_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ postgres:
description: public.points3857.geom
table_source:
content_type: application/x-protobuf
table_source_geog:
content_type: application/x-protobuf
table_source_multiple_geom:
content_type: application/x-protobuf
description: public.table_source_multiple_geom.geom1
Expand Down
18 changes: 18 additions & 0 deletions martin/tests/pg_table_source_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ async fn table_source() {
description: public.points3857.geom
table_source:
content_type: application/x-protobuf
table_source_geog:
content_type: application/x-protobuf
table_source_multiple_geom:
content_type: application/x-protobuf
description: public.table_source_multiple_geom.geom1
Expand All @@ -106,6 +108,22 @@ async fn table_source() {
properties:
gid: int4
");

let source2 = table(&mock, "table_source_geog");
assert_yaml_snapshot!(source2, @r"
schema: public
table: table_source_geog
srid: 4326
geometry_column: geog
bounds:
- -2
- 0
- 142.84131509869133
- 45
geometry_type: Geometry
properties:
gid: int4
");
}

#[actix_rt::test]
Expand Down
3 changes: 3 additions & 0 deletions tests/expected/auto/catalog_auto.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@
"table_source": {
"content_type": "application/x-protobuf"
},
"table_source_geog": {
"content_type": "application/x-protobuf"
},
"table_source_multiple_geom": {
"content_type": "application/x-protobuf",
"description": "public.table_source_multiple_geom.geom1"
Expand Down
13 changes: 13 additions & 0 deletions tests/expected/auto/save_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,19 @@ postgres:
geometry_type: GEOMETRY
properties:
gid: int4
table_source_geog:
schema: public
table: table_source_geog
srid: 4326
geometry_column: geog
bounds:
- -2.0
- 0.0
- 142.84131509869133
- 45.0
geometry_type: Geometry
properties:
gid: int4
table_source_multiple_geom:
schema: public
table: table_source_multiple_geom
Expand Down
13 changes: 13 additions & 0 deletions tests/expected/martin-cp/flat-with-hash_save_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,19 @@ postgres:
geometry_type: GEOMETRY
properties:
gid: int4
table_source_geog:
schema: public
table: table_source_geog
srid: 4326
geometry_column: geog
bounds:
- -2.0
- 0.0
- 142.84131509869133
- 45.0
geometry_type: Geometry
properties:
gid: int4
table_source_multiple_geom:
schema: public
table: table_source_multiple_geom
Expand Down
13 changes: 13 additions & 0 deletions tests/expected/martin-cp/flat_save_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,19 @@ postgres:
geometry_type: GEOMETRY
properties:
gid: int4
table_source_geog:
schema: public
table: table_source_geog
srid: 4326
geometry_column: geog
bounds:
- -2.0
- 0.0
- 142.84131509869133
- 45.0
geometry_type: Geometry
properties:
gid: int4
table_source_multiple_geom:
schema: public
table: table_source_multiple_geom
Expand Down
13 changes: 13 additions & 0 deletions tests/expected/martin-cp/normalized_save_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,19 @@ postgres:
geometry_type: GEOMETRY
properties:
gid: int4
table_source_geog:
schema: public
table: table_source_geog
srid: 4326
geometry_column: geog
bounds:
- -2.0
- 0.0
- 142.84131509869133
- 45.0
geometry_type: Geometry
properties:
gid: int4
table_source_multiple_geom:
schema: public
table: table_source_multiple_geom
Expand Down
46 changes: 46 additions & 0 deletions tests/fixtures/tables/table_source_geog.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
DROP TABLE IF EXISTS table_source_geog;
CREATE TABLE table_source_geog(gid serial PRIMARY KEY, geog geography(GEOMETRY, 4326));

INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(0 0)'::geography);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually prefer to have the same type of elements in a table - if you mix points and linestrings, what would be the type of the MVT data? Not a blocker, but for a test we may want a few more tables like this .... although perhaps one mixed table is also good to test the results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from the table_source test case, which is doing the same for geometries.
I agree that we can probably use a few more tests for the geography columns.
Any recommendations on the best approach for this? I could just copy/paste all the tests we have for geometries to run them on geographies as well, but that feels like a lot of duplication, so I was wondering if there could be a more clever way?

INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(-2 2)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;LINESTRING(0 0, 1 1)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;LINESTRING(2 2, 3 3)'::geography);

INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(30 10)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;LINESTRING(30 10, 10 30, 40 40)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POLYGON((30 10, 40 40, 20 40, 10 20, 30 10))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POLYGON((35 10, 45 45, 15 40, 10 20, 35 10),(20 30, 35 35, 30 20, 20 30))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTIPOINT((10 40), (40 30), (20 20), (30 10))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTIPOINT(10 40, 40 30, 20 20, 30 10)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTILINESTRING((10 10, 20 20, 10 40),(40 40, 30 30, 40 20, 30 10))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTIPOLYGON(((30 20, 45 40, 10 40, 30 20)),((15 5, 40 10, 10 20, 5 10, 15 5)))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTIPOLYGON(((40 40, 20 45, 45 30, 40 40)),((20 35, 10 30, 10 10, 30 5, 45 20, 20 35),(30 20, 20 15, 20 25, 30 20)))'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;GEOMETRYCOLLECTION(POINT(4 6),LINESTRING(4 6,7 10))'::geography);

-- Curves are not supported in geography columns
-- INSERT INTO table_source_geog(geog) values ('SRID=4326;CIRCULARSTRING(1 5, 6 2, 7 3)'::geography);
-- INSERT INTO table_source_geog(geog) values ('SRID=4326;COMPOUNDCURVE(CIRCULARSTRING(0 0,1 1,1 0),(1 0,0 1))'::geography);
-- INSERT INTO table_source_geog(geog) values ('SRID=4326;CURVEPOLYGON(CIRCULARSTRING(-2 0,-1 -1,0 0,1 -1,2 0,0 2,-2 0),(-1 0,0 0.5,1 0,0 1,-1 0))'::geography);
-- INSERT INTO table_source_geog(geog) values ('SRID=4326;MULTICURVE((5 5,3 5,3 3,0 3),CIRCULARSTRING(0 0,2 1,2 2))'::geography);

INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84124343269863 11.927545216212339)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84022627741408 11.926919775099435)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84116724279622 11.926986082398354)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84129834730146 11.926483025982757)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84086326293937 11.92741281580712)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84083973422645 11.927188724740008)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.8407405154705 11.92659842381238)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84029057105903 11.92711170365923)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.8403402985401 11.927568375227375)'::geography);
INSERT INTO table_source_geog(geog) values ('SRID=4326;POINT(142.84131509869133 11.92781306544329)'::geography);

-- DO NOT CREATE INDEX ON GEOGRAPHY COLUMN -- this table is used in a test case

DO $do$ BEGIN
EXECUTE 'COMMENT ON TABLE table_source_geog IS $tj$' || $$
{
"description": null,
"foo": {"bar": "foo"}
}
$$::json || '$tj$';
END $do$;
2 changes: 2 additions & 0 deletions tests/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ test_jsn fnc_comment function_Mixed_Name
kill_process "$MARTIN_PROC_ID" Martin

test_log_has_str "$LOG_FILE" 'WARN martin::pg::query_tables] Table public.table_source has no spatial index on column geom'
test_log_has_str "$LOG_FILE" 'WARN martin::pg::query_tables] Table public.table_source_geog has no spatial index on column geog'
test_log_has_str "$LOG_FILE" 'WARN martin::fonts] Ignoring duplicate font Overpass Mono Regular from tests'
validate_log "$LOG_FILE"
remove_line "${TEST_OUT_DIR}/save_config.yaml" " connection_string: "
Expand Down Expand Up @@ -387,6 +388,7 @@ test_jsn fnc_comment_cfg fnc_Mixed_Name

kill_process "$MARTIN_PROC_ID" Martin
test_log_has_str "$LOG_FILE" 'WARN martin::pg::query_tables] Table public.table_source has no spatial index on column geom'
test_log_has_str "$LOG_FILE" 'WARN martin::pg::query_tables] Table public.table_source_geog has no spatial index on column geog'
test_log_has_str "$LOG_FILE" 'WARN martin::fonts] Ignoring duplicate font Overpass Mono Regular from tests'
validate_log "$LOG_FILE"
remove_line "${TEST_OUT_DIR}/save_config.yaml" " connection_string: "
Expand Down
Loading