Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1364 closed task (fixed)

[raster] Make ST_Union a c-implementation

Reported by: robe Owned by: dustymugs
Priority: medium Milestone: PostGIS 2.1.0
Component: raster Version: trunk
Keywords: history Cc:

Description

Bborie,

Just putting this on the list before I forget to. I'm still revising Pierre's code to get rid or the rasterexp and the infinite number of state functions. I'm almost done but doing some spot checking to make sure my revised looks right.

Hopefully that will make it easier to rewrite when the time comes since there won't be quite so many moving parts.

I was tempted to make it multiband supportable, as now I have to write stupid code like this to reconstitute my chopped pictures and which was even more painful without the array ST_AddBand construct.

SELECT ST_AddBand(NULL, 
ARRAY[ST_Union(ST_Band(rast,1))
    , ST_Union(ST_Band(rast,2))
    , ST_Union(ST_Band(rast,3)) ] )
FROM samples.downtown_chunked 

Change History (18)

comment:1 Changed 5 years ago by robe

Actually we should probably think about how the ST_Union interface will change when we introduce multiband support since if a lot of people start using a particular construct, we can't just pull it out.

I would have liked ST_Union(rast) to just union bands in parallel like I have above or an argument to denote that is the behavior you want with an array of numbers like ST_Band. Perhabs something like a

ST_Union(rast raster,band_nums integer[])

But Pierre has 7 ST_Union aggregate permutations already none of which take a band number.

comment:2 Changed 5 years ago by dustymugs

Multiband ST_Union would be nice but we're not going to have significant performance gains until I rewrite the C 2-raster MapAlgebra?.

7 permutations without a band parameter? Does all versions work on all bands?

comment:3 Changed 5 years ago by robe

No all permutations as far as I can tell assumes the first band is what you want to work with. Actually I haven't tested his other permutations. i was mostly testing with ST_Union(rast) and ST_Union(rast, p_expression) -- the first just defaults to 'LAST' I think.

I'm almost tempted to cut the rest out and just leave maybe two since I think the most common use case for ST_Union is going to be stitching back your chunked rasters based on a spatial filter or grouping which is pretty much satisfied by the first two. Then leave the other permutations up for discussion. I don't think Pierre would like me stripping out 5 of his functions (which of course is more like 20 functions). If I keep all 7, I think I can do it still without introducing the rastexpr and cutting out most of his various permutations of state functions. So it will still be a savings of about 10 functions.

comment:4 Changed 5 years ago by robe

By grouping I meant something like a file_name that raster2pgsql currently offers. That way you can stitch back a chunked dtem tile or something. That I see is the number 2 case next to spatial aggregation. I think all these extra permutations are going to confuse the hell out of people. With that said would be nice to see one version that takes a band number so you don't have to do ST_Band and alter expand that to take an array of bands.

comment:5 Changed 5 years ago by dustymugs

I'm assuming that some of the permutations are duplicates of others as I don't believe Pierre uses default parameter values.

comment:6 Changed 5 years ago by robe

Yah those I took care of to some extent, but AGGREGATES don't seem to support default args fully (you can't define a default arg withint a CREATE AGGREGATE nad the state functions it doesn't seem to want to use one with a default arg that would otherwise match what it needs), so you would end up with more than you normally would with a regular function.

comment:7 Changed 5 years ago by pracine

Milestone: PostGIS Raster FuturePostGIS Future

comment:8 Changed 5 years ago by dustymugs

Summary: [raster] Make ST_Union a c-implementation and also handle multiple bands[raster] Make ST_Union a c-implementation

I've broken this ticket into two due to the two distinct goals. Refer to #2021 for multi-band support in ST_Union.

comment:9 Changed 5 years ago by dustymugs

Milestone: PostGIS FuturePostGIS 2.1.0
Owner: changed from pracine to dustymugs
Status: newassigned

comment:10 Changed 5 years ago by dustymugs

Keywords: history added
Resolution: fixed
Status: assignedclosed

Added in r10344. This C-implementation won't be backported to PostGIS 2.0 as it isn't a bug fix AND the significant amount of underlying code added/removed.

comment:11 Changed 5 years ago by robe

cool can't wait to try this out to compare with my previous workloads

comment:12 Changed 5 years ago by pracine

Great work Bborie!

Did you test performance?

I think the key question is "Is the state function still iterating on all the pixels forming the union of the raster or just on the changing extent?"

comment:13 Changed 5 years ago by dustymugs

It runs on whatever the union's evolving extent needs to be.

comment:14 Changed 5 years ago by pracine

So we should expect O(n2) for a coverage of disjoint tiles? As demonstrated in my blog post? Not much change. Just that C is (supposed to be) faster.

comment:15 Changed 5 years ago by dustymugs

In addition to non-stop deserialization and hexwkb encoding/decoding...

comment:16 Changed 5 years ago by pracine

Do you mean performance is bad?

comment:17 Changed 5 years ago by dustymugs

No. Performance is better because of all the deserialization and hexwkb encoding/decoding that happens to the working union raster with every call to the function doesn't happen anymore with the C variant.

comment:18 Changed 5 years ago by pracine

Ok. But still, time processing should grow by a second degree order with the number of pixel involved?

Note: See TracTickets for help on using tickets.