shp2pgsql gives bad output on bad input

[pramsey]

Using the building_oops data from #167, I get the following output:

Heron-2:buildings_oops pramsey$ shp2pgsql buildings_oops.shp b
Shapefile type: PolygonZ
Postgis type: MULTIPOLYGON[4]
SET STANDARD_CONFORMING_STRINGS TO ON;
BEGIN;
CREATE TABLE "b" (gid serial PRIMARY KEY,
"layer" varchar(254),
"elevation" numeric,
"shape_leng" numeric,
"shape_area" numeric);
SELECT AddGeometryColumn('','b','the_geom','-1','MULTIPOLYGON',4);
ERROR: geometry requires more points
INSERT INTO "b" ("layer","elevation","shape_leng","shape_area",the_geom) VALUES ('S-BUILDING','1384.22300000','0.00000000000','0.00000000000',Heron-2:buildings_oops pramsey$ 

Again, I re-iterate, that we should not be doing validity checking in the loader.

[robe]

+1 Yes I concur - we should not be doing validity checking on load unless we provide a switch to turn it on. This is really something that deserves a switch.

Before you could ignore bad records by removing the begin/commits - am I correct in saying that is not an option any more as the .sql generation will just choke?

[mcayland]

Yeah. The original plan to have a switch to allow erroneus geometries got lost in the discussions about valid/invalid geometries in the database. Given that we didn't decide on an outcome for 1.4, I'd suggest that we should revert to the 1.3 behaviour until we actually come up with a better proposal to move forward.

Patch attached for people to test.

[mcayland]

Remove validity checks from shp2pgsql

[robe]

Mark,

Haven't had time to test this yet. Though looking at it seems like a shame to throw away all that work. Well not throw away but not use it.

I'm still all for a switch and ideally the switch would work something like this

value: 0 (or not specified - same behavior as 1.3.6) value: 1 (the behavior you have currently - just break at first bad) value: 2 (throw out bad entries) (not sure if this would be possible but would solve most of my problems — check but either remark out the bad lines in the generated SQL or put an error on the screen to alert of the bad geometry and leave it out of the generated sql).

The main issue I have is I have lets say 1,000,000 buildings to load in and if I've got a couple of buildings with not enough points to form a polygon — I'm not going to waste time sitting around trying to figure out what they meant by a 2-sided polygon. And its a pain to get rid of begin commits. But its still good to know which records will not come thru in which case I can fudge it and just put in a NULL for the geometry or something.

[robe]

Mark,

I tested this out and it seems okay to me now and did some sample loads. Can you commit the patch and close this out. Can we have an RC1 and plan to do the final release next week or late this week?

Paul — your shp2gui thingy doesn't seem to have an option to output to a file, so since your malformed would never load in the db anyway — I can't easily tell if the patch Mark put in patched the gui as well or not. Though given you can't load a "not enough points" without option to dump to file, I guess it really doesn't matter.

[pramsey]

Aha, this is where having two different pieces of core code finally comes back to bite me. The patch also needs to be applied against the core.o that sites behind the gui/cli executables.

[pramsey]

A patch for the gui/cli fork as well.

[mcayland]

No worries - I'll review and apply these later today.

ATB,

Mark.

[mcayland]

I've created a new ticket for the merge (#204) and assigned it to Paul. This should probably hit 1.5 as soon as we set that up.

ATB,

Mark.

[mcayland]

Committed to trunk as r4203 and 1.4 branch as r4204.

ATB,

Mark.