Skip to content
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 raster type & functions to manage rasters in the spatial extension #298

Open
wants to merge 29 commits into
base: v1.0.0
Choose a base branch
from

Conversation

ahuarte47
Copy link
Contributor

@ahuarte47 ahuarte47 commented Apr 2, 2024

This PR adds support to manage rasters (A wrapper of GDALDataset) in the extension.

It provides a new type RASTER, and a set of functions to load, process and save this data type.

Description of this code here.

@Maxxen
Copy link
Member

Maxxen commented Apr 3, 2024

Hi! Thanks for this PR, it's really cool! Im currently on vacation so I've only skimmed this quickly, but I have some concerns:

  • Using a POINTER for the RASTER type doesn't seem like it actually stores anything in the database? What happens if I load some rasters into a table and restart DuckDB? IIRC PostGIS has the option to either embed the raster into a blob (which limits you to 4gb in duckdbs case) or store a file path to the raster on disk, reopening it when needed.

  • The reason why I have been hesitant to add raster support to spatial is that I don't think DuckDB's vectorized execution engine is a good fit for raster processing. DuckDB's vector engine operates on batches of 2048 elements at a time which is great when all elements are small-ish (the full vector fit into memory), but as rasters generally are much bigger this boon quickly becomes a disatvantage, e.g. a single vector of 512x512x4 tiles is already 2GB of ram (external ram held by GDAL in this case, which DuckDB is unaware of and can't buffer manager). Additionally DuckDB vectors need to be immutable, so if you chain a bunch of operations in order every intermediate vector will be held in memory too for the duration of the pipeline. This whole approach is in stark contrast to e.g. GDALs VRT's that are designed to avoid holding anything in RAM at all, or PostGIS/SpatiaLite where only a single row is processed at a time.

  • A big part of PostGIS raster processing is setting up constraints to make sure mosaics don't have seams, but DuckDB is currently pretty limited when it comes to constraints in general.

  • GDAL has turned out to be a very difficult dependency to wrangle in practice (I think like a 1/3 of all open issues here are related to GDAL filesystem issues and we still have problems in WASM) and I've been trying to slowly lessen our dependency on it by implementing native readers for common vector formats, but its OK now because we strip a lot of the extra drivers and have a build setup that somewhat works even if its very hacky. I suspect with the PR we would have to add back a lot of the raster drivers (and their dependencies in turn).

That said im not entirely against merging this, I just need some time to look it over once I get back to work. As I mentioned I've been toying with the idea of splitting all the GDAL functionality into a separate extension with additional drivers (and caveats) relying on VCPKG to build the dependencies, in which case I think this would fit in great.

@Maxxen
Copy link
Member

Maxxen commented Apr 3, 2024

Also, you need to update the DuckDB submodule as the main CI always builds for latest DuckDB main or switch the target brach to v0.10.1 as that has the latest changes (a big documentation rework) and is pinned to DuckDB v0.10.1.

@ahuarte47
Copy link
Contributor Author

ahuarte47 commented Apr 3, 2024

Thanks @Maxxen for your comments,

* Using a POINTER for the RASTER type doesn't seem like it actually stores anything in the database? What happens if I load some rasters into a table and restart DuckDB? IIRC PostGIS has the option to either embed the raster into a blob (which limits you to 4gb in duckdbs case) or store a file path to the raster on disk, reopening it when needed.

Yes, nothing is stored in the database. I am managing POINTERs (temporary objects) to avoid to load in RAM that so big objects such as rasters. I share your concerns. Maybe this implementation does not fit all use cases you are thinking, I see a raster as temporary object, loaded from external data sources, use them in a query and finally save to other external store if it is necesary. Load & save methods from/to BLOB can exist to read/write the database, but I do not know if this is the way to go.

* GDAL has turned out to be a very difficult dependency to wrangle in practice (I think like a 1/3 of all open issues here are related to GDAL filesystem issues and we still have problems in WASM) and I've been trying to slowly lessen our dependency on it by implementing native readers for common vector formats, but its OK now because we strip a lot of the extra drivers and have a build setup that somewhat works even if its very hacky. I suspect with the PR we would have to add back a lot of the raster drivers (and their dependencies in turn).

Yes, you are right, this PR is adding more dependencies to GDAL, but studing PostGIS source code, many operations are implemented using GDAL as well, I thought this was as a valid solution to do not reinvent wheels. Anyway, we could mitigate this dependency separating this raster support in other new extension, the spatial extension could drop its GDAL dependency when it comes possible.

Anyway, of course, feel free about this MR, this is a PoC, and you feel it raises unacceptable issues to fit in the DuckDB engine model, I am comfortable with that.

@Maxxen
Copy link
Member

Maxxen commented Apr 3, 2024

Alright, let me have another look when I get back.

I think the primary blocker right now is just how to deal with persistence. I agree that it might be a good idea to avoid copying the raster data itself into DuckDB as blobs, but I think there should be some way to "restore" a set of rasters from disk when opening a duckdb database with a raster table (if they are available on disk - otherwise throw an error or return NULL or something, we can see how PostGIS handles it). Maybe by storing a file path or an ID to some other lookup structure instead of the raw pointers.

Regarding GDAL - let's maybe not worry about this too much right now. We already have it as a dependency, and it's probably going to stay for a while. If we just use the built-in raster drivers for now it won't add much complexity to the build.

Thanks again for your work!

@ahuarte47 ahuarte47 changed the base branch from main to v0.10.1 April 4, 2024 00:40
@ahuarte47 ahuarte47 changed the base branch from v0.10.1 to v0.10.2 April 29, 2024 12:54
@ahuarte47 ahuarte47 changed the base branch from v0.10.2 to v1.0.0 June 9, 2024 17:29
@armaanv
Copy link

armaanv commented Jul 18, 2024

is merging this on the roadmap?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants