fix optimize_expression to genereate atlas of feature with textual PKs #72
Conversation
maxencelaurent
commented
Jun 23, 2024
maxencelaurent
commented
- allow optimization of non-numeric PK expressions
- do not assume PK is the very first field of the list: use layer.primaryKeyAttributes() instead
- fixes GetPrintAtlas does not work if ID of feature is not a number #71
- related to [Bug]: GetPrintAtlas does not work if id of feature is not a number lizmap-web-client#4534
* allow non-numeric PK expression optimization * do not assume PK is the very first field of the list: use layer.primaryKeyAttributes() instead * fixes 3liz#71 * related to 3liz/lizmap-web-client#4534
|
This PR is breaking tests. |
* using string pkey is now safe * According to the [PyQGIS documentation](https://qgis.org/pyqgis/3.0/core/Vector/QgsVectorLayer.html#qgis.core.QgsVectorLayer.primaryKeyAttributes), `QgsVectorLayer.primaryKeyAttributes` represents the indexes of the attributes which make up the layer's primary key. Former `test_tools::test_optimize_filter` defined this using fieldnames. This has been fix using field indexes. + New test case in which the primary key is not the first attribute of the layer
|
I've fixed the test. There were two issues. First was quite straightforward: it's now safe to use string PK. The second was that the test layers were poorly crafted. |
Yes you are right, but having a list of field indexes or a list of field strings, it didn't change anything the code, as only the length of the list was used. So it was somehow readable. But yes, it's ok. |
| expression = expression.replace('$id', '"{}"'.format(pk_field.name())) | ||
| logger.info('$id has been replaced by "{}" in layer "{}"'.format(pk_field.name(), layer.id())) |
There was a problem hiding this comment.
| expression = expression.replace('$id', '"{}"'.format(pk_field.name())) | |
| logger.info('$id has been replaced by "{}" in layer "{}"'.format(pk_field.name(), layer.id())) | |
| expression = expression.replace('$id', f'"{pk_field.name()}"') | |
| logger.info(f'$id has been replaced by "{pk_field.name()}" in layer "{layer.id()}"') |
Let's switch to fstrings now at the same time
| primary_keys = layer.primaryKeyAttributes() | ||
| if len(primary_keys) != 1: | ||
| logger.info("Primary keys are not defined in the layer '{}'.".format(layer.id())) | ||
| return expression | ||
|
|
||
| field = layer.fields().at(0) | ||
| if not field.isNumeric(): |
There was a problem hiding this comment.
Thanks for your proposal. I am not sure to understand correctly the context you are targeting with this PR.
It seems to me $id will always return an integer, the internal layer feature ID, which:
- corresponds exactly to the integer primary key of the table if the field type is integer.
- does not correspond to the primary key value if the field type is text (or other even bigint, for big datasets, $id will return the feature line number as it is returned, for example for a big PostgreSQL table).
For example, $id will return the value of id_integer_pk for this layer:
| id_integer_pk | name |
|---|---|
| 1 | one |
| 2 | two |
but will not return the value of id_text_pk for this layer, but 1 or 2:
| id_text_pk | name |
|---|---|
| AB | one |
| CD | two |
Can you please share a QGIS project with an example layer (FlatGeoBuf, GeoJSON, etc.), so that we can test it on real data ?
There was a problem hiding this comment.
Hi @mdouchin
Please find some data here: https://github.com/maxencelaurent/qgis-atlasprint-issue-71
As you mentioned, there are two cases:
In the first one, QGIS is able to retrieve the PK from existing fields. Thus, layer.primaryKeyAttributes() returns the list of indices of the fields that make up the PK. LWC use this information to build the GetPrintAtlas queries: EXP_FILTER="$id IN ('68ca5ce1-7ac4-4fbe-9a76-3c0d3d41fa53')"
Under some circumstances (eg. postgresql), such a field may be of type UUID. This behaviour almost only exists for postgresql layers. I couldn't reproduce it with sqlite for example.
In the second one, QGIS is not able to retrieve the PK from existing fields. Hence, it uses an internal $id field which is always of type integer. layer.primaryKeyAttributes() always returns an empty list. LWC uses this @id to build the query : EXP_FILTER="$id IN (1)".
On the atlas-plugin side, the EXP_FILTER is optimized. The purpose of this optimization is to replace the $id with the effective field name of the PK.
The plugin verifies that :
- The PK is made of exactly one field
- The first field of the layer is numeric
If both tests are true, the filter is optimized : $id is replaced with the name of the first field.
In my data, you'll find some layers:
text_pk_first_place & text_pk_sec_place
- SQLite layer wtih text PK
- QGIS failed to retrieve PK
⚠️ - interal
$idis used. - The EXP_FILTER is not optimized
- GetPrintAtlas works fine.
int_pk_first_place
- SQLite layer wtih int PK
- QGIS uses effective PK
- The EXP_FILTER is optimized.
- GetPrintAtlas works fine ✅
int_pk_sec_place
- SQLite layer wtih int PK
- QGIS uses effective PK
- The EXP_FILTER is not optimized
⚠️ - GetPrintAtlas works fine
uuid_pk_first_place & uuid_pk_sec_place
- SQLite layer wtih uuid PK
- QGIS failed to retrieve PK
⚠️ - interal
$idis used. - The EXP_FILTER is not optimized.
- GetPrintAtlas works fine.
int_pk_sec_place_but_other_int_first
- SQLite layer wtih integer PK
- the first field if not the PK
- the first field is a number
- QGIS uses effective PK
- The EXP_FILTER is badly optimized
⚠️ - GetPrintAtlas fails 💀 💥
psql_uuid
- PostgreSQL layer wtih uuid PK
- QGIS uses effective PK
- The EXP_FILTER is not optimized.
- GetPrintAtlas fails 💀 💥
