[QGIS-Developer] OGR: sqlite with subset query, QgsOgrFeatureIterator::fetchFeature with SpatialFilterRect always returns feature with id 0

[Nyall Dawson]

On 28 May 2018 at 10:43, Nyall Dawson <nyall.dawson at gmail.com> wrote:
> On 4 September 2017 at 18:56, Sandro Mani <manisandro at gmail.com> wrote:
>>
>> On 03.09.2017 23:50, Even Rouault wrote:
>>
>>> Ok, so if we only do this for non-select queries, we should be safe?
>>
>>
>>
>> Yes. I suspect the case where a full SELECT statement is used are rare, and
>> people able to do that can tweak the request. It would be unreliable to try
>> modifying it.
>>
>>
>>
>>>
>>
>>> > > Something like this [1] seems to fix the issue, don't know if it's the
>>
>>> > >
>>
>>> > > best way forward though?
>>
>>> >
>>
>>> > I've left a few comments in the commit
>>
>>>
>>
>>> Many thanks. I've pushed a revised version at [1].
>>
>>
>>
>> I've added a couple suggestions for more robustness, but looks good to me.
>>
>> Thanks, filed PR: https://github.com/qgis/QGIS/pull/5122
>
> I've been tracking down a performance regression in 3.0 and have
> tracked it back to this PR.
>
> What's happening is that in
> https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrfeatureiterator.cpp#L204
> , everytime a new feature is requested with a particular ID the ENTIRE
> layer is being read until that feature is found! This destroys QGIS
> performance when more than a handful of feature IDs are requested -
> e.g. try setting a subset string on a layer, then selecting a couple
> of hundred points and zooming to selection.
>
> I've been trying to find a good solution to this without luck - if I
> use OGR_L_SetAttributeFilter to specifically request the desired
> feature with orig_ogr_fid equal to the requested feature id, then it's
> a bit better. There's no longer the need to iterate through all the
> features, but it's still very very slow compared to a direct
> OGR_L_GetFeature call.
>
> Even do you have any good tricks we could utilise here to speed this
> up? Any other pointers for approaches to test?

Actually - unless I'm missing something, I don't believe that the
whole condition at
https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrfeatureiterator.cpp#L204
is even required, because it appears that OGR_L_GetFeature works
correctly with the feature IDs taken from the original fid column. In
other words, when ExecuteSQL is used, we can't safely rely on
OGR_F_GetFID to return a usable feature id and have to use the ogc_fid
attribute value instead... BUT when requesting a feature from the
layer by ID, we CAN safely use OGR_L_GetFeature with IDs taken from
ogc_fid and be confident that we get the desired feature returned.

Is that interpretation correct? If so, this fix is implemented in:
https://github.com/qgis/QGIS/pull/7091

Nyall