Skip to content

Add geometrycolumn validation warning for GeoJSON write function #114

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add geometrycolumn validation warning for GeoJSON write function #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0b3cc5
Initial plan
Copilot Aug 31, 2025
360888e
Add geometrycolumn validation warning for non-geometry column names
Copilot Aug 31, 2025
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
1 change: 1 addition & 0 deletions src/io.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ function _lower(obj; geometrycolumn = first(GI.geometrycolumns(obj)))
end
elseif Tables.istable(obj)
!(geometrycolumn isa Symbol) && throw(ArgumentError("GeoJSON.jl can only write a single geometry column which must be specified as a `Symbol`, but was passed `$(geometrycolumn)` instead."))
geometrycolumn != :geometry && @warn "GeoJSON specification requires the geometry column to be named 'geometry'. The column '$(geometrycolumn)' will be written as 'geometry' in the output."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
geometrycolumn != :geometry && @warn "GeoJSON specification requires the geometry column to be named 'geometry'. The column '$(geometrycolumn)' will be written as 'geometry' in the output."
geometrycolumn != :geometry && @warn "GeoJSON specification requires the geometry column to be named `geometry`. The column '$(geometrycolumn)' will be written as 'geometry' in the output." maxlog=10

Copy link
Member

@asinghvi17 asinghvi17 Aug 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if we want this to warn, actually...it seems like a legitimate usecase. @copilot give a boolean option to turn this off as a kwarg, default to false. @visr what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer errors (or no errors) instead of warnings. In this case I'd throw an error since the users wants something that is invalid. We could just also just update the line above.

Last time I was bitten by @warn was 3 days ago: apache/arrow-julia#559

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, my concern is that if you read something from e.g. a database with a geometry column called :MYGEOM, and you now have that in memory as a DataFrame, then you would have to make the change manually rather than simply specifying geometrycolumn to write.

Copy link
Member

Choose a reason for hiding this comment

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

I see, and what about just ignoring it? The docstring is already pretty clear:

you may pass a Symbol to the geometrycolumn keyword argument,
to indicate which column of the table holds the geometries. Note that this will not keep the name,
the geometry must be written to the :geometry column of the GeoJSON according to the spec.

According to this description it is about identifying the right column in the table, which is a useful feature for :MYGEOM.

Actually the default of first(GI.geometrycolumns(obj)) can perhaps also be surprising. If there is more than one you probably want people to choose and not pick the first.

geom_col_idx = Tables.columnindex(obj, geometrycolumn)
# There is a strange bug where Tables.columnnames on some tables is empty,
# so we use the schema instead
Expand Down
10 changes: 10 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,16 @@ include("geojson_samples.jl")
@test t2_geojson.prop1 == getproperty.(t2, :prop1)
@test t2_geojson.prop2 == getproperty.(t2, :prop2)

@testset "geometrycolumn warnings" begin
# Test warning is shown for non-geometry column
t_warn = [(somethingelse=(1.0, 2.0), prop1=1)]
@test_logs (:warn, r"GeoJSON specification requires.*geometry.*somethingelse.*") GeoJSON.write(t_warn; geometrycolumn = :somethingelse)

# Test no warning for geometry column
t_no_warn = [(geometry=(1.0, 2.0), prop1=1)]
@test_logs GeoJSON.write(t_no_warn; geometrycolumn = :geometry)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@test_logs GeoJSON.write(t_no_warn; geometrycolumn = :geometry)
@test_nowarn GeoJSON.write(t_no_warn; geometrycolumn = :geometry)

end

@testset "Metadata" begin
@test GI.DataAPI.metadatasupport(typeof(t2_geojson)) == (; read = true, write = false)
@test GI.DataAPI.metadatakeys(t2_geojson) == ("GEOINTERFACE:geometrycolumns", "GEOINTERFACE:crs")
Expand Down
Loading