Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add info about usable "expression dialect" to filter dialog #58730

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kannes
Copy link
Contributor

@kannes kannes commented Sep 13, 2024

The user can enter query expressions in the filter/query dialog, but often people get confused as to why some expressions work but others don't.

Random examples:

This change hopefully makes it more obvious what kind of expressions are supported. Uses the new data provider methods introduced in #58781

Compiled and tested manually with several layer types, but not all.

Screenshots

Screenshot_20240918_171856
Screenshot_20240918_171840
Screenshot_20240918_171831
Screenshot_20240918_171808
Screenshot_20240918_171755


Old screenshots from an earlier state:


Developer time sponsored by WhereGroup GmbH and Covid.

@github-actions github-actions bot added this to the 3.40.0 milestone Sep 13, 2024
@kannes kannes changed the title Add provider and storage type into to filter dialog Add provider and storage type to filter dialog Sep 13, 2024
Copy link

github-actions bot commented Sep 13, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 2d44364)

@nyalldawson nyalldawson added the Freeze Exempt Feature Freeze exemption granted label Sep 13, 2024
@NyakudyaA
Copy link
Contributor

NyakudyaA commented Sep 13, 2024

Can we not also add a link to each provider type documentation to allow users to see navigate to the documentation page so that they can explore available options with a specific provider i.e
"The available options depends on the provider type and storage options. Please consult the provider documentation to see supported options i.e. For Geopackage https://gdal.org/en/latest/drivers/vector/gpkg.html#sql"

@nyalldawson
Copy link
Collaborator

There's actual gdal API to retrieve the SQL dialect in use - see OSGeo/gdal#6154

That would be more appropriate to use here

@kannes
Copy link
Contributor Author

kannes commented Sep 14, 2024

That looks great!

Can I simply assume that QGIS is built with GDAL 3.6+?

I will create the URLs and then link the different GDAL_DMD_SUPPORTED_SQL_DIALECTS to them.

@kannes
Copy link
Contributor Author

kannes commented Sep 14, 2024

Nice, GDAL also provides GDAL_DMD_HELPTOPIC with the path to the help file, e.g. "drivers/vector/ili.html"

My current state:

image
image
image

I am linking to /latest/ on the GDAL homepage, is that ok? I did not find version-specific docs.

The code is probably a total mess, I will look at it again tomorrow. Any change recommendations are welcome of course :)

I think removing the provider and storage type again would be good.

@agiudiceandrea
Copy link
Contributor

Can I simply assume that QGIS is built with GDAL 3.6+?

INSTALL.md says: GDAL/OGR >= 3.2.0
qgis.org currently provides also QGIS for Ubuntu jammy with GDAL 3.4.1, Debian bullseye with GDAL 3.2.2, and macOS with GDAL 3.3.2.

src/gui/qgsquerybuilder.cpp Outdated Show resolved Hide resolved
src/gui/qgsquerybuilder.cpp Outdated Show resolved Hide resolved
@kannes
Copy link
Contributor Author

kannes commented Sep 17, 2024

Thank you for the feedback!

But I realised something and am not sure if this is going into a good direction because I introduced a "display of potential SQL dialects one could utilize if using GDAL directly" vs. what I really wanted which is "display of the SQL dialect in use by the provider filter".

The ILIKE operator is often the cause for confusion and frustration for users so I use it as example here. ILIKE is usable at least with PostgreSQL sources and - if one uses OGR SQL.

But I just realised that even if a layer has OGRSQL support, it will not necessarily be used. E.g. a GeoPackage will have NATIVE, OGRSQL and SQLITE and if I write a query involving ILIKE (e.g. "NAME" ILIKE 'foo' on world.gpkg, it will error:

  • GUI says "An error occurred when executing the query, please check the expression syntax."
  • Debug log says "ERROR 1: sqlite3_prepare_v2(SELECT Count(*) FROM "countries" WHERE "NAME" ILIKE 'foo') failed: near "ILIKE": syntax error"

I assume that the native dialect is preferred (which makes great sense) but I could not figure it out from the sources.

So now I worry if this PR would introduce more confusion than help... It should display the dialect that is actually used, instead of the dialects that the user could use if they were using GDAL directly. As far as I know there is no way for the user to specify the SQL dialect to use, so theoretical support is irrelevant and misleading.

So I would rather remove the dialects display again.


(Ideally the user could select the utilized dialect themselves, e.g. with a dropdown list. But that would require much more work.)

src/gui/qgsquerybuilder.cpp Outdated Show resolved Hide resolved
src/gui/qgsquerybuilder.cpp Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator

@kannes

display of the SQL dialect in use by the provider filter".

The first value returned from the metadata value is always the default, which will be the one QGIS uses

@nyalldawson
Copy link
Collaborator

@kannes I've taken the opportunity to add proper API for this up at the QgsDataProvider level -- see #58781 . I'd suggest rebasing this PR when #58781 is merged and then using those methods in the dialect, as it will get you nice strings + urls for all providers which support subseting.

@kannes
Copy link
Contributor Author

kannes commented Sep 18, 2024

Oh fantastic, thank you! I will try to finish this asap.

@kannes kannes force-pushed the show_providerinfos_in_filterdialog branch from 50b4474 to 304848b Compare September 18, 2024 16:03
@kannes kannes changed the title Add provider and storage type to filter dialog Add info about usable "expression dialect" to filter dialog Sep 18, 2024
@kannes
Copy link
Contributor Author

kannes commented Sep 18, 2024

Done! This is really cool, thank you so much.

The text above the expression widget now just reads "Enter a [...] to filter the layer", where the appropriate name will be inserted as a hyperlink to the URL.

I decided to still add a visible information about the provider in use to the dialog, now right after the layer name at the topmost label.

Showing the storage type too seems not that useful to me anymore so I have not kept that.

New screenshots are in the main post above.

@nyalldawson are you sure that those dialects & URLs are all correct? For WFS it says "WFS query expression" and links to https://docs.ogc.org/is/09-025r2/09-025r2.html#83 but I have never seen a XML string used for a subset string. And for filtering WFS on load, the SQL Query composer exists, which uses SQL again (https://gis.stackexchange.com/questions/236847/tutorial-for-wfs-connection-sql-query-composer)

@rouault
Copy link
Contributor

rouault commented Sep 18, 2024

For WFS it says "WFS query expression" and links to https://docs.ogc.org/is/09-025r2/09-025r2.html#83 but I have never seen a XML string used for a subset string

the WFS provider can understand a subset of SQL WHERE clause or SQL SELECT as valid subsetStrings. I can't think of existing documentation where this clearly defined

Examples from unit tests:

        vl.setSubsetString('intfield = 2')
        vl.setSubsetString(
            "SELECT o.*, m.id AS m_id FROM \"my:typename\" m JOIN \"my:othertypename\" o ON m.id = o.main_id WHERE m.id > 0 ORDER BY m.id DESC")

@Gustry Gustry added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Sep 19, 2024
@qgis-bot
Copy link
Collaborator

@kannes

This pull request has been tagged for the changelog.

  • The description will be harvested so please provide a "nearly-ready" text for the final changelog
  • If possible, add a nice illustration of the feature. Only the first one in the description will be harvested (GIF accepted as well)
  • If you can, it's better to give credits to your sponsor, see below for different formats.

You can edit the description.

Format available for credits
  • Funded by NAME
  • Funded by URL
  • Funded by NAME URL
  • Sponsored by NAME
  • Sponsored by URL
  • Sponsored by NAME URL

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog Items that are queued to appear in the visual changelog - remove after harvesting Freeze Exempt Feature Freeze exemption granted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants