Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#318 closed task (fixed)

[REVIEW] PostGIS provider patch 3.4 from Bruno Scott

Reported by: mloskot Owned by: mloskot
Priority: critical Milestone: 3.4.0
Component: PostGIS Provider Version:
Severity: 1 Keywords: postgis patch
Cc: bscott@… External ID:

Description

This task is about reviewing Bruno Scott's big patch for the PostGIS provider posted to the list - PATCH : PostGIS provider patch 3.4

Here is list of tickets fixed by this patch:

#94 Generate extent for features assigned to default spatial context

#106 PostGIS provider cannot delete a feature class

#171 Fdo Postgis Autogenated identity property is mandatory

#178 PostGIS : Can't insert in a non-feature class

#232 Fdo Postgis null and not null filter does not work

#233 Fdo Postgis in and not in filter does not work

#234 Fdo Postgis currently does not support anything but lowercase identifiers

#235 Fdo Postgis Exception with insert

#236 Fdo Postgis does not support non spatial classes

#241 Implement Support for SelectAggregates?, SpatialExtents? and Count

#310 PostGIS provider : change class naming convention without ~

#311 PostGIS provider : mismatch between FdoGeometricTypes? and FdoGeometryType?

#312 PostGIS provider : remove boost_date_time dll dependency

#313 PostGIS provider : can't filter,insert or update date/datetime

#314 PostGIS provider : check-out/check-in crashes Autodesk Map

mloskot: Big thanks to Bruno for this patch!

Attachments (1)

postgis_trunk-bruno_scott.patch (107.3 KB) - added by mloskot 12 years ago.
PostGIS provider big patch from Bruno Scott

Download all attachments as: .zip

Change History (20)

Changed 12 years ago by mloskot

PostGIS provider big patch from Bruno Scott

comment:1 Changed 12 years ago by mloskot

Severity: 31

comment:2 Changed 12 years ago by mloskot

Status: newassigned

comment:3 Changed 12 years ago by mloskot

In the patch for #310, there is one place unclear to me, where original version (line 228) is:

-        FdoStringP fdoClassName = FdoStringP::Format(L"%s~%s",
-            static_cast<FdoString*>(stReader->GetSchemaName()),
-            static_cast<FdoString*>(stReader->GetTableName()));

and patched version (line 213) is:

+        //fdoClassName = FdoStringP::Format(L"%s~%s",
+        //    static_cast<FdoString*>(stReader->GetSchemaName()),
+        //    static_cast<FdoString*>(stReader->GetTableName() ));

In both versions the format is the same and uses old ~.

comment:4 Changed 12 years ago by mloskot

Bruno, do you mind if I'd suggest to remove comments about ticket number, what was fixed, etc. after the patch is applied?

For example, here the first line seems redundant to me (after the patch is applied to the FDO tree):

/// ticket #310 - class naming convention wihout ~ - eric barby 
/// Set schema name (physical schame of postgres). 
FDOPOSTGIS_API void SetSchemaName(FdoString* value); 

comment:5 Changed 12 years ago by mloskot

Patch for ticket #312 (remove boost_date_time dll dependency) looks good.

comment:6 Changed 12 years ago by mloskot

AFAIU, the patch for ticket #314 (check-out/check-in crashes Autodesk Map) is a temporary fix. If that's right, I'd suggest to keep the #314 open.

comment:7 in reply to:  6 Changed 12 years ago by bscott

According to the ticket 314 description "check-out/check-in crashes Autodesk Map" Map does not crash anymore so i think we should close that ticket and create a new ticket

"PostGIS provider does not support Transaction"

comment:8 Changed 12 years ago by mloskot

Bruno,

I see you've added two new files:

Providers/PostGIS/Src/Provider/PgTablesReader.h
Providers/PostGIS/Src/Provider/PgTablesReader.cpp

Certainly, I'm completely fine with them. I just need to ask you to fix the copyright notice by replacing the 2nd line:

// Copyright (C) 2007 Refractions Research, Inc. 

to read

// Copyright (C) 2008 ...

where ... is name of the author of this file or company.

Please, tell me what to use as (C) and I'll apply the fix.

comment:9 Changed 12 years ago by mloskot

Bruno,

Regardless of my question in the 3rd comment above, I'm removing these 3 lines which are commented and use old formatting string with tilde.

comment:10 in reply to:  8 Changed 12 years ago by bscott

Actually these two files are not really new one. Providers/PostGIS/Src/Provider/PgTablesReader.h Providers/PostGIS/Src/Provider/PgTablesReader.cpp They are just PgSpatialTablesReader?.* renamed to PsTablesReader?.* as they not only read spatial table but also non spatial tables. It makes thing clearer :)

comment:11 Changed 12 years ago by mloskot

Bruno,

Ah, got it. I've mistaken it because the patch does not include delete action. OK, clear.

comment:12 Changed 12 years ago by mloskot

Bruno,

Is this intentional that in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp, lines 49-51:

// eric barby - faster way of describing schema 
SchemaDescription *schemaDesc = mConn->DescribeSchema(); 
if (!schemaDesc || !schemaDesc->IsDescribed())  

schemaDesc is not wrapped with FdoPtr? ?

The Connection::DescribeSchema? increments reference counter to the schema description object. So, client of the schemaDesc is supposed to decrement it, ideally using automatic collection with FdoPtr?.

comment:13 Changed 12 years ago by mloskot

Just FYI, it's recommended to not to use vendor-specific API, but to use portable replacements like: prefer FdoCommonOSUtil::wcsicmp instead of _wcsicmp.

comment:14 Changed 12 years ago by mloskot

In file Providers/PostGIS/Src/Provider/InsertCommand.cpp, line 195, there is following comment:

// eric barby - need to parse date properly 

Is this a kind of TODO saying that the code below should be improved in future or it's just a description of the code below as the actual improvement?

comment:15 Changed 12 years ago by mloskot

Bruno,

I'd have a suggestion to mark lines that require some future review/fixes/improvements/etc. with well-known markers like:

XXX
TODO
FIXME
NOTE

Here, I'm referring to your comment in Providers/PostGIS/Src/Provider/InsertCommand.cpp, line 204:

// bruno scott - geom is not used anywhere, maybe we should delete this line 

Markers simply help to grep through the sources and look for stuff that needs some work. Also, marked lines are displayed in the Tasks List window in Visual Studio :-)

comment:16 Changed 12 years ago by mloskot

Hmm, I've encountered a few replacements like this:

- std::string sqlSelect; 
+ std::string sqlSelect("");

Actually, these lines have the same effect, but I'm just curious: Have you experienced problems with improperly default constructed std::string so you decided to explicitly initialize it? If yes, what compiler?

comment:17 in reply to:  12 Changed 12 years ago by mloskot

Replying to mloskot:

Bruno,

Is this intentional that in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp, lines 49-51:

Actually this question applies to {Insert|Update}Command.cpp files.

comment:18 Changed 12 years ago by mloskot

Resolution: fixed
Status: assignedclosed

IMPORTANT

The patch has been applied to the trunk (r3904).

Big thanks to Bruno, Eric and their Team for the fantastic work. I believe I can close this ticket. Please, reopen if needed.

comment:19 in reply to:  12 Changed 12 years ago by mloskot

Bruno,

Sorry for bothering, could you review lines 49-51 in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp why the schema description is not grabbed with FdoPtr??

Note: See TracTickets for help on using tickets.