-
Notifications
You must be signed in to change notification settings - Fork 59
Add GeoParquet support #727
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.
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 GeoParquet support #727
Conversation
…uet-support # Conflicts: # yarn.lock
Integration tests report: appsharing.space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really neat! I just have some minor suggestion. Also, would you like to add an example file under examples/
so I can test it?
case 'GeoParquetSource': { | ||
const parameters = source.parameters as IGeoParquetSource; | ||
|
||
const geojson = await loadFile({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel great about converting geoparquet to geojson as we lose all the performance advantages. Openlayers doesn't support geoparquet (yet? there is no open issue in their GitHub repo... should we make one?), so we need to convert to something... what about flatgeobuf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at flatgeobuf, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elifsu-simula have you had any luck on this?
I also see your PR has quite a lot of conflicts now (mainly due to the changes in imports I assume). I can help with this if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Martin! Sorry for the late reply. I have been on vacation. I am starting to look into this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to convert GeoParquet files to FlatGeobuf using GDAL's ogr2ogr function, but our current gdal3.js WASM build doesn’t include the Parquet driver. I’ve been working on rebuilding it with the Parquet driver, but before investing more time I wanted to check if this approach is feasible or if I should look into other ways without modifying the original gdal3.js build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does OpenLayers support FlatGeobuf as-is?
If we start investigating making our own gdal WASM build, I'd rather prefer it being done on emscripten-forge, that way we could also use it in the Python kernel in JupyterLite if we also compile the Python bindings for it.
But that's probably not a straightforward task. I personally would be ok with the current state of the PR as a first solution, then track in an issue exploring other approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenLayers doesn't support FlatGeobuf, but the FlatGeobuf project has an OpenLayers integration (example) . Then I'll fix the conflicts and create an issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat
Hi @elifsu-simula I am testing your pr , the file isn't opening here is a video : ndd.mp4Below is the screenshot of the error I get in the terminal : Maybe the problem is on my part but it seems to be connected to file encoding? |
Change the video to show that other file are still opening. |
Hi! Previously, I added an example parquet file, now I replaced it with a JGIS file where the parquet layer is already added. |
Hi thanks, so we can't open directly geoparquet file with this pr ? |
No I didn't add this functionality. We can only add GeoParquet layers to the JGIS documents from the widget or Python API. Do we want to directly open them as well? |
OK I see no problem , I thought so because before the example was directly geoparquet file. |
It is working on my side now , @martinRenou will probably look into it after , I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
Solves #647
Added support for GeoParquet data format, both to UI and to Python API, using the library geoparquet.
I added a test for Python API but didn't add a UI test. I also did not add support for exporting GeoParquet layers to QGIS. If asked, I can do these either in this PR or another PR.
Checklist
Resolves #XXX
.Failing lint checks can be resolved with:
pre-commit run --all-files
jlpm run lint
📚 Documentation preview: https://jupytergis--727.org.readthedocs.build/en/727/
💡 JupyterLite preview: https://jupytergis--727.org.readthedocs.build/en/727/lite