geo/geotransform: implement ST_Transform#49783
Conversation
|
This change is |
1134802 to
e7ed835
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan)
pkg/geo/geoproj/geoproj.go, line 33 at r1 (raw file):
// goToCSlice returns a CR_PROJ_Slice from a given Go byte slice. func goToCSlice(b []byte) C.CR_PROJ_Slice {
Can we define the string and slice c++ structs and the conversions once, and use them in all the library wrappers we are writing? Or is that tricky in our current build?
pkg/geo/geoproj/geoproj.go, line 73 at r1 (raw file):
} if err := cStatusToUnsafeGoBytes(C.CR_PROJ_Transform( goToCSlice([]byte(from)),
isn't []byte(from) a copy? If so, should we change Proj4Text to be a []byte and avoid this copy.
pkg/geo/geoproj/proj.h, line 18 at r1 (raw file):
// CR_PROJ_Slice contains data that does not need to be freed. // // It can be either a Go or C pointer (which indicates who allocated the
nit: extra //
pkg/geo/geoproj/proj.h, line 25 at r1 (raw file):
} CR_PROJ_Slice; typedef CR_PROJ_Slice CR_PROJ_Status;
are all the status strings char* literals?
pkg/geo/geoproj/proj.h, line 27 at r1 (raw file):
typedef CR_PROJ_Slice CR_PROJ_Status; CR_PROJ_Status CR_PROJ_Transform(CR_PROJ_Slice from, CR_PROJ_Slice to,
can you add a function comment clarifying that the points are (x[i], y[i], z[i]) for 0 <= i < point_coint
Also, the from, to here and in the caller Project is a bit confusing -- it suggests that to is being written to. How about something like fromSpec, toSpec.
pkg/geo/geoproj/proj.cc, line 20 at r1 (raw file):
namespace { CR_PROJ_Status CR_PROJ_ErrorFromErrorCode(int code) {
nit: I thought we'd decided stylistically to use char* err (same for other similar code in this file) when doing GEOS (geos.h mostly follows that style though there is some inconsistency in recent additions).
pkg/geo/geoproj/proj.cc, line 35 at r1 (raw file):
CR_PROJ_Status err = {.data = NULL, .len = 0}; auto ctx = pj_ctx_alloc(); auto fromPJ = pj_init_plus_ctx(ctx, std::string(from.data, from.len).c_str());
is this expensive to initialize each time? it needs to parse a structured string at least.
And we could avoid the copy needed for c_str by storing null terminated strings in ProjInfo
pkg/geo/geoprojbase/.clang-format, line 1 at r1 (raw file):
---
where did the contents of this file come from, and where is the C++ code that this is formatting -- I don't see any non-Go files in this directory.
pkg/geo/geoprojbase/geoprojbase.go, line 26 at r1 (raw file):
AuthName string AuthSRID int SRText string
can you add comments for AuthName, AuthSRID, SRText
pkg/geo/geoprojbase/projections.go, line 27 at r1 (raw file):
}, 4004: { SRID: 4004,
is 4004 a popular one?
pkg/geo/geotransform/geotransform.go, line 108 at r1 (raw file):
numCoords := len(flatCoords) / layout.Stride() xCoords := make([]float64, numCoords) yCoords := make([]float64, numCoords)
numCoords may be small, so it may be worthwhile doing a single slice allocation and using different parts of it for x, y, z.
pkg/geo/geotransform/geotransform.go, line 115 at r1 (raw file):
yCoords[i] = flatCoords[base+1] if layout.ZIndex() != -1 { zCoords[i] = flatCoords[base+layout.ZIndex()]
// In this case ZIndex() is 2
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/geo/geoproj/geoproj.go, line 33 at r1 (raw file):
Previously, sumeerbhola wrote…
Can we define the string and slice c++ structs and the conversions once, and use them in all the library wrappers we are writing? Or is that tricky in our current build?
i think it's tricky to find the right incantations in our current build and i'd rather avoid it for now.
pkg/geo/geoproj/geoproj.go, line 73 at r1 (raw file):
Previously, sumeerbhola wrote…
isn't
[]byte(from)a copy? If so, should we changeProj4Textto be a[]byteand avoid this copy.
Done.
pkg/geo/geoproj/proj.h, line 25 at r1 (raw file):
Previously, sumeerbhola wrote…
are all the status strings
char*literals?
Yep!
pkg/geo/geoproj/proj.h, line 27 at r1 (raw file):
Previously, sumeerbhola wrote…
can you add a function comment clarifying that the points are (x[i], y[i], z[i]) for 0 <= i < point_coint
Also, the
from,tohere and in the callerProjectis a bit confusing -- it suggests thattois being written to. How about something likefromSpec,toSpec.
Done.
pkg/geo/geoproj/proj.cc, line 20 at r1 (raw file):
Previously, sumeerbhola wrote…
nit: I thought we'd decided stylistically to use
char* err(same for other similar code in this file) when doing GEOS (geos.h mostly follows that style though there is some inconsistency in recent additions).
copied the .clang-format file to fix this.
pkg/geo/geoproj/proj.cc, line 35 at r1 (raw file):
Previously, sumeerbhola wrote…
is this expensive to initialize each time? it needs to parse a structured string at least.
And we could avoid the copy needed for
c_strby storing null terminated strings inProjInfo
Done.
pkg/geo/geoprojbase/geoprojbase.go, line 26 at r1 (raw file):
Previously, sumeerbhola wrote…
can you add comments for AuthName, AuthSRID, SRText
Done.
pkg/geo/geoprojbase/projections.go, line 27 at r1 (raw file):
Previously, sumeerbhola wrote…
is 4004 a popular one?
no idea, the only popular latlng one i know is 4326. the survey says 3857 was most popular, which wasn't latlng. i picked an arbitrary one.
pkg/geo/geotransform/geotransform.go, line 108 at r1 (raw file):
Previously, sumeerbhola wrote…
numCoordsmay be small, so it may be worthwhile doing a single slice allocation and using different parts of it for x, y, z.
Done.
pkg/geo/geotransform/geotransform.go, line 115 at r1 (raw file):
Previously, sumeerbhola wrote…
// In this case ZIndex() is 2
Done.
pkg/geo/geoprojbase/.clang-format, line 1 at r1 (raw file):
Previously, sumeerbhola wrote…
where did the contents of this file come from, and where is the C++ code that this is formatting -- I don't see any non-Go files in this directory.
this was in the wrong dir -- moving to ./pkg/geo/geoproj
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 10 of 10 files at r2.
Reviewable status:
pkg/geo/geoproj/proj.h, line 27 at r1 (raw file):
(reminder)
can you add a function comment clarifying that the points are (x[i], y[i], z[i]) for 0 <= i < point_coint
pkg/geo/geoprojbase/geoprojbase.go, line 47 at r2 (raw file):
type ProjInfo struct { SRID geopb.SRID // AuthName is authority who has provided this projection (e.g. ESRI, EPSG).
nit: ... the authority ...
This PR implements ST_Transform, allowing the transformation from one SRID to another. The `geoprojbase` package defines a barebones set of types as well as a hardcoded list of SRIDs to keep in memory. I've only filled in a few for now, and will save updating this for a later PR. `geoproj` is strictly a C library interface library which performs the necessary transformations. `geotransform` is where the function is actually handled and to be used by `geo_builtins.go`. Release note (sql change): Implemented the ST_Transform function for geometries.
|
bors r=sumeerbhola |
Merge conflict (retrying...) |
Build succeeded |
This PR implements ST_Transform, allowing the transformation from one
SRID to another.
The
geoprojbasepackage defines a barebones set of types as well as ahardcoded list of SRIDs to keep in memory. I've only filled in a few for
now, and will save updating this for a later PR.
geoprojis strictly a C library interface library which performs thenecessary transformations.
geotransformis where the function is actually handled and to be usedby
geo_builtins.go.Resolves #49055
Resolves #49056
Resolves #49057
Resolves #49058
Release note (sql change): Implemented the ST_Transform function for
geometries.