[QGIS-Developer] QGIS/OGR: FeatureIds reassigned on write to data provider?

[Sandro Mani]

On 06.07.2017 11:00, Sandro Mani wrote:
>
>
> On 06.07.2017 10:57, Even Rouault wrote:
>>
>> On jeudi 6 juillet 2017 10:45:21 CEST Sandro Mani wrote:
>>
>> > On 05.07.2017 14:56, Sandro Mani wrote:
>>
>> > >> That could potentially be done using the current OGR API. Basically
>>
>> > >> in pseudo code, to execute before executing REPACK
>>
>> > >>
>>
>> > >> OGR_L_ResetReading(layer)
>>
>> > >>
>>
>> > >> char** ignored_fields = CSLAddString(NULL, "OGR_GEOMETRY" );
>>
>> > >>
>>
>> > >> OGR_L_SetIgnoredFields( layer, ignored_fields); // for performance.
>>
>> > >>
>>
>> > >> CSLDestroy(ignored_fields);
>>
>> > >>
>>
>> > >> OGR_L_SetAttributeFilter( layer, NULL )
>>
>> > >>
>>
>> > >> OGR_L_SetSpatialFilter( layer, NULL )
>>
>> > >>
>>
>> > >> std::map<GIntBig, GIntBig> mapOldIdToNewId;
>>
>> > >>
>>
>> > >> GIntBig newId = 0;
>>
>> > >>
>>
>> > >> while( feature = OGR_L_GetNextFeature(layer) )
>>
>> > >>
>>
>> > >> {
>>
>> > >>
>>
>> > >> mapOldIdToNewId[OGR_F_GetFID(feature)] = newId;
>>
>> > >>
>>
>> > >> newId ++;
>>
>> > >>
>>
>> > >> OGR_F_destroy(feature);
>>
>> > >>
>>
>> > >> }
>>
>> > >>
>>
>> > >> OGR_L_SetIgnoredFields( layer, NULL );
>>
>> > >
>>
>> > > Ok cool, that sounds like a plan - I'll give it a shot.
>>
>> >
>>
>> > Hmm so while on the provider side it works well, for the geometry
>>
>> > checker it is turning out to be pretty hard to deal with the changing
>>
>> > feature ids (just to cite one example: error fixed by merging geometry
>>
>> > of feature 10 with that of 11, results in {deleted: 10, updated: 11},
>>
>> > but after the feature id adjustment this would read {deleted: 10,
>>
>> > updated: 10}, meaning one would need to keep track that deleted: 10
>>
>> > refers to the old featureid). Not saying that it isn't doable, but the
>>
>> > complexitiy of properly handling the feature id changes is non-trivial.
>>
>> >
>>
>> > So, other suggestion: any objections if I add a method to
>>
>> > QgsVectorDataProvider to temporarily freeze repacking? I could also add
>>
>> > a notification informing the user that the shapefile should not be used
>>
>> > in other applications while repacking is frozen.
>>
>> My feeling is that QgsOgrDataProvider is already complicated enough 
>> with its existing tricks. I'm not sure this temporary freeze 
>> repacking is a right move (how would you decide when you do it ? and 
>> I'm pretty sure users will not get it, or will have issues if they 
>> start an algorithm with an external tool that requires packed shapefiles)
>>
> It would be something I would call explicitly while the geometry 
> checker is active, I think it is safe to assume that users don't 
> expect that they can operate on the shapefile while the geometry 
> checker is processing it.  It would not add much complexity to the 
> provider, just a flag that repacking is frozen, and a setter to set 
> that flag. When the flag is set to false, the shapefile is repacked.
>
I did some playing around and came up with some FeatureId mapping code 
which is pretty space efficient (basically requiring just 
3*nDeletedFeatures entries), see below.

The code however assumes that the gaps in the fid-sequence are filled by 
decreasing the fids after the gap, which may be pretty specific to the 
OGR/Shapefile behaviour. Hence in my view such code should really belong 
into the OGR provider (or perhaps even ogr itself) and not in the 
plugin. Thoughts?

Sandro

-------

#include <algorithm>
#include <iostream>
#include <QMap>
#include <QVector>

typedef int QgsFeatureId;


class FeatureMapping  {

public:
   QgsFeatureId stableToProvider(QgsFeatureId stableId) const
   {
     if(mDeletedStableIds.contains(stableId)){
       return -1;
     }
     // providerId = stableId - nDeletedBeforeStableId
     int nDeletedBeforeStableId = std::lower_bound(mDeletedStableIds.begin(), mDeletedStableIds.end(), stableId) - mDeletedStableIds.begin();
     return stableId - nDeletedBeforeStableId;
   }

   QgsFeatureId providerToStable(QgsFeatureId providerId) const
   {
     int i = 0, n = mProviderToStableOffsetKeys.size(), offset = 0;
     while(i < n && mProviderToStableOffsetKeys[i] <= providerId) {
       offset += mProviderToStableOffsetValues[i++];
     }
     return providerId + offset;
   }

   void updateFeatureIdMap(const QList<QgsFeatureId> &deletedIds)
   {
     for(QgsFeatureId fid : deletedIds) {
       mDeletedStableIds.append(providerToStable(fid));
     }
     std::sort(mDeletedStableIds.begin(), mDeletedStableIds.end());
     int n = mProviderToStableOffsetKeys.size();
     for(QgsFeatureId fid : deletedIds) {
       int pos = std::lower_bound(mProviderToStableOffsetKeys.begin(), mProviderToStableOffsetKeys.end(), fid) - mProviderToStableOffsetKeys.begin();
       if(pos < n && mProviderToStableOffsetKeys[pos] == fid) {
         mProviderToStableOffsetValues[pos] += 1;
       } else {
         mProviderToStableOffsetKeys.insert(pos, fid);
         mProviderToStableOffsetValues.insert(pos, 1);
         ++n;
       }
       if(pos + 1 != n && mProviderToStableOffsetKeys[pos+1] -1 == fid) {
         mProviderToStableOffsetValues[pos] += mProviderToStableOffsetValues[pos+1];
         mProviderToStableOffsetKeys.removeAt(pos + 1);
         mProviderToStableOffsetValues.removeAt(pos + 1);
         --n;
       }
       ++pos;
       while(pos < n) {
           mProviderToStableOffsetKeys[pos++] -= 1;
       }
     }
     std::cout << "offsets = {";
     for(int i = 0, n = mProviderToStableOffsetKeys.size(); i < n; ++i) {
       std::cout << mProviderToStableOffsetKeys[i] << ": " << mProviderToStableOffsetValues[i] << ", ";
     }
     std::cout << "\b\b}" << std::endl;
   }
   int numDeleted() const{ return mDeletedStableIds.size(); }
private:
   QVector<QgsFeatureId> mProviderToStableOffsetKeys;
   QVector<int> mProviderToStableOffsetValues;
   QVector<QgsFeatureId> mDeletedStableIds;
};

void printMapping(const FeatureMapping& mapping) {
   std::cout << "------" << std::endl;
   for(int i = 0; i < 10; ++i) {
     std::cout << i << " -> " << mapping.stableToProvider(i) << std::endl;
   }
   std::cout << "------" << std::endl;
   for(int i = 0, n = 10 - mapping.numDeleted(); i < n; ++i) {
     std::cout << mapping.providerToStable(i) << " <- " << i << std::endl;
   }
   std::cout << "------" << std::endl;
}

int main(int argc, char* argv[]){
   FeatureMapping mapping;

   printMapping(mapping);
   while(true) {
     std::cout << "> ";
     std::string command;
     std::getline(std::cin, command);
     if(command.empty()) {
       continue;
     }else if(command[0] == 'q') {
       break;
     }else if(command[0] == 'd') {
       QgsFeatureId num = std::atoi(command.substr(1).c_str());
       std::cout << "* Delete: stable " << num << ", provider: " << mapping.stableToProvider(num) << std::endl;
       mapping.updateFeatureIdMap({mapping.stableToProvider(num)});
       printMapping(mapping);
     } else {
       std::cout << "Unknown command" << std::endl;
     }
   }

   return 0;
}



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/qgis-developer/attachments/20170710/c57458cf/attachment-0001.html>