duckdb / duckdb_spatial

MIT License
492 stars 41 forks source link

Add raster type & functions to manage rasters in the spatial extension #298

Open ahuarte47 opened 7 months ago

ahuarte47 commented 7 months ago

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 commented 7 months ago

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:

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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!

armaanv commented 4 months ago

is merging this on the roadmap?

ahuarte47 commented 1 month ago

Hi everyone. I am thinking about this old PR. Maybe is it better to manage a raster as other tabular Dataframe? Something similar how RasterFrames is managing them?

A raster could be serialized as a table of chunks or tiles. A parquet (Rasquet :-)) con several metadata columns (CRS, BBOX width, height, datatype, ...) and 1-N band columns with the binary for each raster band.

frafra commented 2 weeks ago

Hi everyone. I am thinking about this old PR. Maybe is it better to manage a raster as other tabular Dataframe? Something similar how RasterFrames is managing them?

I really appreciate your effort, and I haven't replied before because it is challenging for me to understand how such a decision would affect how the data would be queried. Could you provide an example?

A common need that I have when dealing with vector/tabular data is to add some information to them based on some rasters, such as DTM. Would the new approach make it radically different? If that is not the case, using a more common/popular way to handle raster might be better, since people would be already familiar with that way of working.

Thanks again for this great PR :) We really need to have a way to connect raster data to duckdb.

ahuarte47 commented 1 week ago

Hi, the idea is to split the input raster when loading into a collection of small tiles distributed by overlays, and manage them as a table for rows with several metadata columns (size, crs, pixel_type...) and binary columns for each exiting band in the original raster.

This approach is radically different than using the original input raster as a unique source. Managing rasters as collection of tiles can be better for the internal vector engine that duckdb implements, but I think it makes much difficult to manage the tiles when you want to perform raster algebra on them.

I think the current approach is simpler, knowing that duckdb can not use its vector engine to parallelize the computing of queries from rasters.

Anyway, it would be great a debate about what approach seems better.