feat: geoarrow export #30

jorisvandenbossche · 2024-10-09T09:02:19Z

Similar to #28, just opening to be able to add some comments / discuss the design.

For now it just can process points, without any options (but the points do work, as being tested with benbovy/spherely#53

src/s2geography/geoarrow.cc

jorisvandenbossche · 2024-10-09T09:26:07Z

src/s2geography/geoarrow.cc

+    case OutputType::kPoints:
+      impl_->Init(GEOARROW_TYPE_INTERLEAVED_POINT, options, out_schema);
+      break;


I just added kPoints (mapping to interleaved points) for a quick test. I assume we might want to add some generic "native geoarrow" option like kGeoArrow(Interleaved), and then infer the schema from the geographies? (with something similar like GeoArrowGEOSSchemaCalculator you did in geoarrow-c-geos)

It might still be worth allowing the user to specify the type themselves to avoid inference / choose a specific type (e.g. always the multi-version regardless of presence of any multi geom)

geoarrow-c-geos also has a function to help you make a schema:

https://github.com/geoarrow/geoarrow-c-geos/blob/33ad0ba21c76c09e9d72fc4e4ae0b9ff9da61848/src/geoarrow_geos/geoarrow_geos.h#L122-L123

The right entry point I think is still ArrowSchema* (until a point where geoarrow-c exposes an ABI for specifying geometry type constraints, which is I think a ways off). It's definitely good to let the user specify this (often they already know because they're calculating a centroid or something).

The calculator is a definite must! It can probably use geography->num_shape() and geography->dimension():

s2geography/src/s2geography/geography.h

Lines 32 to 48 in 829a0fe

virtual int dimension() const {

if (num_shapes() == 0) {

return -1;

}

int dim = Shape(0)->dimension();

for (int i = 1; i < num_shapes(); i++) {

if (dim != Shape(i)->dimension()) {

return -1;

}

}

return dim;

}

// The number of S2Shape objects needed to represent this Geography

virtual int num_shapes() const = 0;

jorisvandenbossche · 2024-10-09T09:40:31Z

src/s2geography/geoarrow.cc

+void Writer::Init(OutputType output_type, const ImportOptions& options,
+                  struct ArrowSchema* out_schema) {


In the case of initializing the writer with just a type, I added an ArrowSchema as output argument, because for exporting the array to another producer, we need both the array and schema.

I put this above, too, but I think you want a const ArrowSchema* as your entry point into the Writer (you can expose a function to generate the appropriate ArrowSchema* so that those APIs are decoupled).

Good point, if we have a separate function to construct this schema from a type / encoding (or those inferred from an array of gemetries), then it is perfectly fine to only have the Init here starting from a schema

src/s2geography/geoarrow.cc

jorisvandenbossche · 2024-10-19T12:41:08Z

Pushed a small update:

Changed the interface to have a scalar WriteGeography(const Geography& geog) and a separate Finish(struct ArrowArray* out), as discussed
Did not yet update the Init() to just take a schema (and provide some helpers to create / infer the schema)
Added support for Polylines, still have to add Polygon and Collection

jorisvandenbossche · 2024-10-19T12:51:55Z

src/s2geography/geoarrow.cc

+      for (int i = 0; i < poly->num_vertices(); i++) {
+        S2LatLng ll(poly->vertex(i));
+        coords[0] = ll.lng().degrees();
+        coords[1] = ll.lat().degrees();
+        GEOARROW_RETURN_NOT_OK(visitor_.coords(&visitor_, &coords_view_));
+      }


Currently for geometries with >1 coordinates (here for the example of a linestring), I loop over the s2points and call the coords() callback multiple times.

The alternative is to build up a single GeoArrowCoordView with the n_coords set to something larger than 1, and then only do a single call to coords().

Now, given that we don't start from a contiguous array of coords, I suppose it will (performance wise) not matter that much if we would do the conversion to a single array ourselves and do a single coords() call, or or call coords() multiple times and let the underlying writer do the conversion?
Especially since we don't know for sure if the output actually needs single buffers (like for WKT/WKB it would iterate over the coords pairs anyway then, when passing a single CoordView).
Unless the typicaly coords() visitor implementation might have a bunch of overhead per call? In that case it might be beneficial to limit the number of calls to it (I suppose I should just test the difference .. ;))

It's probably better to limit the calls to the coords() callback if it's easy to do it (but not necessary for this PR as long as it is correct!). On other end, we also have to make a copy of the coordinates we send via the callback (i.e., to populate the requisite double[]), so you don't want to send all of them at once. One other slight wrench in that is that some visitor implementations can "return early" (e.g., the WKT formatter, which has a character limit after which it returns EAGAIN to force the reader to stop). As a data point, geoarrow-c's WKB reader buffers 3072 ordinates (had to be divisible by 2, 3, and 4) at a time:

https://github.com/geoarrow/geoarrow-c/blob/2890982956a9f715339abb5e24059300bb147107/src/geoarrow/wkb_reader.c#L103-L111

Adding the ability to transmit more than one coordinate on this callback was one of the things I changed between wk's design (early in my career writing any C code whatsoever) and this visitor. I also never measured the difference comprehensively though...in theory this is in such a tight loop that the function pointer call does matter.

OK, then I think I will leave it at a coords() call per point for this PR, and we can optimize that later

paleolimbot

Just a note that some basic tests are going to be needed at some point. At least for this PR, I think you could replace the WKTWriter and add a few cases to the "wkt roundtrip" tests. The nice part about the visitor pattern is that you can basically just check WKT output and in theory everything else "just works" (or is a problem with code that isn't yours 🙂 ).

jorisvandenbossche · 2024-10-25T15:26:55Z

Just a note that some basic tests are going to be needed at some point.

Yes I know ;) For now I have been testing locally directly with wiring it up in spherely and so tests in python, and this confirms the basics are working. But definitely will have to add tests in s2geography as well!

jorisvandenbossche · 2024-10-25T20:25:49Z

src/vendored/geoarrow/geoarrow.c

-  writer->precision = 16;
-  private->precision = 16;
-  writer->use_flat_multipoint = 1;
-  private->use_flat_multipoint = 1;
+  // TODO should be changed upstream to make this configurable
+  writer->precision = 6;
+  private->precision = 6;


@paleolimbot what would you recommend here?

Pass that as an extra param to GeoArrowArrayWriterInitFromSchema, which can then pass it down here?

Or a setter after the object has been initialized? (I suppose that is more backwards compatible)

Ah! Hang tight, I'll fix that.

geoarrow/geoarrow-c#101

Ok, this is merged into s2geography (or should be!)

#36

jorisvandenbossche · 2024-10-25T20:28:00Z

OK, I deleted most of the WKT writer implementation (which I essentially had moved to the geoarrow writer already, updating the "handler" pattern for "visitor" code), and so that this now reuses the geoarrow based writer, like is done for the WKTReader as well.

And expanded the WKT roundtrip tests which thus indirectly test the GeoArrow::Writer

jorisvandenbossche · 2024-10-26T09:16:27Z

src/s2geography/geoarrow.cc

+    S2LatLng ll(loop->vertex(loop->num_vertices() - 1));
+    coords_[0] = ll.lng().degrees();
+    coords_[1] = ll.lat().degrees();
+    GEOARROW_RETURN_NOT_OK(visitor_.coords(&visitor_, &coords_view_));


@paleolimbot In the case of a hole, the code here (which I essentially took from the wkt writer) explicitly repeats the first coordinate pair (i.e. in the last vertex because we are iterating reversed, so we started with the last).

But for VisitLoopShell above, the code does not do that (it just iterates over the vertices, and s2 doesn't duplicate the first/last).

In practice, though (at least in case of WKT), it seems both work fine, and so I assume that the visitor implemented by GeoArrowArrayWriter (or for WKT the GeoArrowWKTWriter used under the hood) checks if the first coordinate was repeated, and if not repeats it themselves? But is that meant to be a guarantee? Or should ideally the one calling the visitor ensure they are providing closed ring coordinates?

OK, nevermind, I just noticed that in the case for the shell, we are looping until i <= loop->num_vertices() and not until num_vertices() - 1. So the loop.vertex(i) call wraps around, and so we are actually explicitly adding the first coordinate as well
(that trick just not works when iterating in reverse)

I remember fiddling a lot with that, but I think it exploits the special behaviour of vertex():

https://github.com/google/s2geometry/blob/ca1f3416f1b9907b6806f1be228842c9518d96df/src/s2/s2loop.h#L185-L194

Indeed, that's the explanation I also had found

paleolimbot · 2024-10-28T16:19:53Z

src/s2geography/geoarrow.cc

+    S2LatLng ll(loop->vertex(loop->num_vertices() - 1));
+    coords_[0] = ll.lng().degrees();
+    coords_[1] = ll.lat().degrees();
+    GEOARROW_RETURN_NOT_OK(visitor_.coords(&visitor_, &coords_view_));


I remember fiddling a lot with that, but I think it exploits the special behaviour of vertex():

https://github.com/google/s2geometry/blob/ca1f3416f1b9907b6806f1be228842c9518d96df/src/s2/s2loop.h#L185-L194

paleolimbot · 2024-10-28T16:24:54Z

src/s2geography/geoarrow.cc

+        GEOARROW_DIMENSIONS_XY));
+
+    for (const auto& child_geog : geog.Features()) {
+      auto child_point = dynamic_cast<const PointGeography*>(child_geog.get());


These dynamic casts are necessary given the current design of the Geography but hopefully I'll find a way to clean that up (maybe making an internal but virtual Visit(void* visitor) method on the Geography).

For spherely, we recently added the type as essentially an attribute on the spherely::Geography C++ class (i.e. which otherwise wraps the s2geography::Geography class): https://github.com/benbovy/spherely/blob/4e39dee1bf46c9591c68bd2b64541ff7c2f8b14f/src/geography.cpp#L116-L162, so whenever a Geography object gets constructed we also store the type (https://github.com/benbovy/spherely/blob/4e39dee1bf46c9591c68bd2b64541ff7c2f8b14f/src/geography.hpp#L55-L59). Not sure if something similar could/should be moved upstream at some point.

(the clone method a bit higher up in the file is certainly a candidate of moving into s2geography)

paleolimbot · 2024-10-28T16:26:44Z

src/s2geography/geoarrow.h

+
+  void Init(OutputType output_type, const ExportOptions& options);
+
+  void WriteGeography(const Geography& geog);


Do you also need a WriteNull()?

Do you also need a WriteNull()?

That's a good point (for spherely actually not yet, because we don't yet support missing values, but we should support it here in general).
Add a separate method for it, or allow the geog to be a nullptr? (and check that inside WriteGeography?)

Probably a separate method!

paleolimbot · 2024-10-28T18:37:19Z

src/vendored/geoarrow/geoarrow.c

-  writer->precision = 16;
-  private->precision = 16;
-  writer->use_flat_multipoint = 1;
-  private->use_flat_multipoint = 1;
+  // TODO should be changed upstream to make this configurable
+  writer->precision = 6;
+  private->precision = 6;


geoarrow/geoarrow-c#101

jorisvandenbossche · 2024-10-29T17:34:54Z

src/s2geography/wkt-writer_test.cc

  WKTWriter writer_default;
-  EXPECT_EQ(writer_default.write_feature(*geog), "POINT (0 3.333333333333334)");
+  EXPECT_EQ(writer_default.write_feature(*geog), "POINT (0 3.3333333333333344)");

  WKTWriter writer_6digits(6);
-  EXPECT_EQ(writer_6digits.write_feature(*geog), "POINT (0 3.33333)");
-
+  EXPECT_EQ(writer_6digits.write_feature(*geog), "POINT (0 3.333333)");


So this is the main difference I see compared to the previous WKTWriter implementation (maybe because of using ryu?), so that the significant digits are interpreted differently.

(actually, the significant_digits name is then not really correct any more, and should we use precision here as well like is used in geoarrow-c?)

Yes, precision is a better name. I think the previous version was using the sprintf() (or maybe the ostream)-based implementation, which uses slightly different words for all of that.

paleolimbot

Thank you for this!

There are a few follow-ups (null writing, maybe more efficient coordinate things, tessellation, more comprehensive testing), but I think those are more effectively handled one at a time based on this work.

jorisvandenbossche · 2024-10-29T18:16:16Z

There are a few follow-ups (null writing, maybe more efficient coordinate things, tessellation, more comprehensive testing), but I think those are more effectively handled one at a time based on this work.

Yep, indeed, and for the tessellation I already have a WIP branch

Will just rename the keyword to precision

jorisvandenbossche · 2024-10-29T18:27:26Z

OK, all green!

paleolimbot · 2024-10-29T18:33:44Z

(I think you have write access too? Feel free to merge things!)

jorisvandenbossche · 2024-10-29T18:35:02Z

I think you have write access too?

I think I forgot to accept your invite and it expired .. :)

feat: geoarrow export

2288f40

jorisvandenbossche marked this pull request as draft October 9, 2024 09:04

jorisvandenbossche mentioned this pull request Oct 9, 2024

feat: Add GeoArrow export #28

Closed

jorisvandenbossche commented Oct 9, 2024

View reviewed changes

src/s2geography/geoarrow.cc Outdated Show resolved Hide resolved

jorisvandenbossche commented Oct 9, 2024

View reviewed changes

some clenaup

269e5b9

jorisvandenbossche force-pushed the geoarrow-writer branch from 5fb0170 to 269e5b9 Compare October 9, 2024 09:38

jorisvandenbossche commented Oct 9, 2024

View reviewed changes

paleolimbot reviewed Oct 9, 2024

View reviewed changes

src/s2geography/geoarrow.cc Outdated Show resolved Hide resolved

jorisvandenbossche added 3 commits October 19, 2024 13:15

use scalar WriteGeography function with separate Finish function

76456b2

add polyline visitor

963d602

fix multipoint, set lower precision for WKT

74c1e09

jorisvandenbossche commented Oct 19, 2024

View reviewed changes

add polygon and collection visitors

066561a

paleolimbot reviewed Oct 25, 2024

View reviewed changes

jorisvandenbossche added 2 commits October 25, 2024 22:08

replace WKT writing, split options

8957918

cleanup

082a929

jorisvandenbossche commented Oct 25, 2024

View reviewed changes

jorisvandenbossche added 3 commits October 26, 2024 10:45

clean-up

198e85e

Merge remote-tracking branch 'upstream/main' into geoarrow-writer

8911808

add brief doc comment

4a7292b

jorisvandenbossche commented Oct 26, 2024

View reviewed changes

paleolimbot reviewed Oct 28, 2024

View reviewed changes

jorisvandenbossche mentioned this pull request Oct 28, 2024

feat: Allow precision to be set in GeoArrowArrayWriter geoarrow/geoarrow-c#101

Merged

jorisvandenbossche added 2 commits October 29, 2024 16:46

Merge remote-tracking branch 'upstream/main' into geoarrow-writer

1a8c8c0

wire up significant_digits option for WKT

b868526

jorisvandenbossche marked this pull request as ready for review October 29, 2024 17:28

cleanup

7fa6b90

jorisvandenbossche commented Oct 29, 2024

View reviewed changes

paleolimbot approved these changes Oct 29, 2024

View reviewed changes

significant_digits -> precision

0704e44

paleolimbot merged commit 79dce6e into paleolimbot:main Oct 29, 2024
4 checks passed

jorisvandenbossche deleted the geoarrow-writer branch October 29, 2024 18:34

jorisvandenbossche mentioned this pull request Oct 29, 2024

feat: add projection and tessellation options to GeoArrow writer #37

Draft

	virtual int dimension() const {
	if (num_shapes() == 0) {
	return -1;
	}

	int dim = Shape(0)->dimension();
	for (int i = 1; i < num_shapes(); i++) {
	if (dim != Shape(i)->dimension()) {
	return -1;
	}
	}

	return dim;
	}

	// The number of S2Shape objects needed to represent this Geography
	virtual int num_shapes() const = 0;

		void Writer::Init(OutputType output_type, const ImportOptions& options,
		struct ArrowSchema* out_schema) {


		void Init(OutputType output_type, const ExportOptions& options);

		void WriteGeography(const Geography& geog);

feat: geoarrow export #30

feat: geoarrow export #30

Conversation

jorisvandenbossche commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 29, 2024

jorisvandenbossche commented Oct 29, 2024

paleolimbot commented Oct 29, 2024

jorisvandenbossche commented Oct 29, 2024

jorisvandenbossche commented Oct 9, 2024 •

edited

Loading

jorisvandenbossche commented Oct 25, 2024 •

edited

Loading