Aside: The test configuration is getting a little wonky with things like:

test:
	./env/bin/pytest tests/test_lib --cov=h3 --cov=tests/test_lib --cov-fail-under=100

	./env/bin/pip install cython
	./env/bin/cythonize tests/test_cython/cython_example.pyx
	./env/bin/pytest tests/test_cython --cov=tests/test_cython

It will probably be easy for the makefile and the github actions files to get out of sync. Hopefully we can get back to a single test command when we figure out the Cython compilation issue and can have everything at 100% coverage.

Looks good so far.

I'm reviewing this on mobile, so I likely missed it...

Are we checking that the input geometry is valid? Invalid geometry could lead to invalid memory access (unless h3 fixed this recently).

Are we checking that the input geometry is valid? Invalid geometry could lead to invalid memory access (unless h3 fixed this recently).

I don't think we currently do a ton of validation, but it it something we could consider adding more of in the future.

The H3 C library does some, and can halt and return an error code, but I'm not sure of the extent of validation it does. Maybe @nrabinowitz or @isaacbrodsky would be more familiar?

@jmealo, are you aware of any good test cases? Or maybe examples of other libraries that do the kind of validation you're thinking of?

It was my understanding that this specific function was marked experimental in the h3 library because it failed fuzzing/valgrind-type tests.

In PostGIS we're going to be calling either ST_IsValid or ST_MakeValid to make it safe to call. In my testing, the overhead was extremely negligible.

For the memory safety and avoiding undefined behavior benefits alone it seems well worth it.

IIRC that function comes from GDAL. If the validation happens in Python, I would consider adding a boolean flag skipValidation that defaults to false.

This would be safe for folks who don't RTFM and for folks who do, provided they validate geometry elsewhere, no additional performance overhead.

I would add an unsafe_ prefix or explicitly name the function and input parameters to make it abundantly clear to only pass previously validated geometry or use a boolean flag to skip validation that defaults to false.

Edited: Clarified thoughts on safety versus performance.

It was my understanding that this specific function was marked experimental in the h3 library because it failed fuzzing/valgrind-type tests.

The fuzzer issue related to underestimating the size of the output buffer with maxPolygonToCellsExperimentalSize. The function was made safe by passing in the size of the output buffer. This allows the function to detect that it would write past the end of buffer and return an error if so. There should not be any outstanding fuzzer or valgrind failures on this function. (There may be a few timeouts.)