Connections api results widget #44051

elpaso · 2021-07-05T07:27:54Z

Description

This is the implementation of QGIS Enhancement: Port DB Manager Table Management Functionalities to Browser: SQL execution (part 3) qgis/QGIS-Enhancement-Proposals#205

Features

SQL dialog available from browser panel for:
- DB connections
- Schema items
- Table items
Multi-threaded implementation for both API token fetching and row fetching: no GUI blocking
Fully interruptible API
Progressive loading of features in the results view (fetchMore API)

nyalldawson · 2021-07-05T22:02:43Z

Looking great! Can I suggest a small refinement to the fetch more behaviour? I think good logic could be:

Fetch the first 200 rows only upfront
On the first request to fetch more, start retrieving ALL the remaining rows

This would give users an opportunity to see if they've made an error in their SQL immediately before we go ahead and fetch all the results (potentially avoiding unwanted expensive database work), but also allow the full advantage of your threaded model to fetch all results so that they're ready for the user "in advance". Make sense?

src/app/browser/qgsinbuiltdataitemproviders.cpp

src/core/providers/ogr/qgsgeopackageproviderconnection.cpp

src/gui/qgsqueryresultwidget.cpp

src/gui/qgsqueryresultwidget.h

elpaso · 2021-07-06T07:32:36Z

Looking great! Can I suggest a small refinement to the fetch more behaviour? I think good logic could be:
1. Fetch the first 200 rows only upfront

2. On the first request to fetch more, start retrieving ALL the remaining rows
This would give users an opportunity to see if they've made an error in their SQL immediately before we go ahead and fetch all the results (potentially avoiding unwanted expensive database work), but also allow the full advantage of your threaded model to fetch all results so that they're ready for the user "in advance". Make sense?

I would rather prefer to leave it like it is now, the reason is that fetching ALL rows in case of a huge DB could crash QGIS due to out of memory conditions.

Probably we could achieve the best of both solutions by pre-fetching a certain number of features and keeping them in a cache but I'm not sure it worths the added complexity.

Error in spatial index exists check.

src/app/browser/qgsinbuiltdataitemproviders.cpp

src/core/providers/ogr/qgsgeopackageproviderconnection.cpp

src/core/providers/qgsabstractdatabaseproviderconnection.h

src/core/qgis.h

nyalldawson · 2021-07-06T22:54:22Z

I would rather prefer to leave it like it is now, the reason is that fetching ALL rows in case of a huge DB could crash QGIS due to out of memory conditions.

Fair enough. The thing I'm not so taken with is the progress bar shown in the GUI which only progresses as the table is scrolled. To me that's a bit odd UI -- I could imagine a user sitting and waiting for ever for the progress bar to complete, thinking that the query is still running and that they need to wait before they can do anything. Maybe just remove the bar entirely?

elpaso · 2021-07-07T06:46:32Z

I would rather prefer to leave it like it is now, the reason is that fetching ALL rows in case of a huge DB could crash QGIS due to out of memory conditions.

Fair enough. The thing I'm not so taken with is the progress bar shown in the GUI which only progresses as the table is scrolled. To me that's a bit odd UI -- I could imagine a user sitting and waiting for ever for the progress bar to complete, thinking that the query is still running and that they need to wait before they can do anything. Maybe just remove the bar entirely?

You are right, to explain the genesis of the progress bar:

I first introduced it to make it clear that something is happening (indeterminate state) while performing the query execution (the first step)
the original implementation was fetching all rows (0-100 %, standard progress bar)

Now, I'd like to keep the progress bar while doing the query execution in step 1. For the fetching step I'm ok, to leave the feature fetched counter and remove the progress bar (or maybe just show-hide it briefly when the actual fetching is happening, it may be slow on certain connections).

About the execution time of step 1, we need to show it all the times because the step 2 starts immediately after step 1 is completed.

nyalldawson · 2021-07-07T07:04:15Z

@elpaso

Now, I'd like to keep the progress bar while doing the query execution in step 1. For the fetching step I'm ok, to leave the feature fetched counter and remove the progress bar (or maybe just show-hide it briefly when the actual fetching is happening, it may be slow on certain connections).

Sounds good to me!

nyalldawson · 2021-07-11T10:02:38Z

Great work Alessandro -- you should be very proud of this one!!

elpaso · 2021-07-11T16:54:57Z

@nyalldawson thank you for reviewing this and for the valuable advice!

stefanuhrig · 2021-08-04T13:44:31Z

src/providers/hana/qgshanaproviderconnection.cpp

@@ -52,6 +54,12 @@ bool QgsHanaProviderResultIterator::hasNextRowPrivate() const
  return mNextRow;
 }

+long long QgsHanaProviderResultIterator::rowCountPrivate() const
+{
+  // TODO: hana team, this is for you.


@mrylov Can you take care of the implementation and check whether we have or should implement additional methods?

Sure, I will take a closer look.

sigeal · 2022-03-24T09:00:10Z

Can you please have a look at this issue :
#47906

elpaso added Feature QGIS Browser labels Jul 5, 2021

github-actions bot added this to the 3.22.0 milestone Jul 5, 2021

nyalldawson reviewed Jul 6, 2021

View reviewed changes

elpaso added 24 commits July 6, 2021 20:52

QgsQueryResultWidget

d914a50

Message bar and other improvements: working!

079264e

Query result widget files and tests

cc709a0

Remove debug output

d958722

SQL results widget: add query layers support (initial)

b988ca8

Move to thread the first phase of the query (before the iterator)

9fc81ff

Start new layer group box collapsed

40785b5

Better error handling/reporting

1a94b07

Rebase

4201942

Rebase 2

e1a8ef5

Fix crash QsciAPIs is destroyed while worker running

9adbdfa

Notify error on SQL layer creation

94056c8

Doxy

af44d35

Create SQL layer and widget config based on enum

cf26022

Handle fid and geom cols in GPKG exec sql

34c5cdc

Comments on fid issue

fd793dc

Comment method

7b202fe

GPKG/Spatialite: return fid and geom from selects

2c123dc

More tests for execSql

5d5e828

Fix crash when connection is invalid (nullptr)

ebfed52

Better comment

af9febc

Provider QSCI APIs for sqlite and PG

27eb574

Remove method and fix widget hiding

dfc5648

Add tooltips

5ba105f

elpaso added 10 commits July 6, 2021 21:00

Doxy

3cb3b8f

Spell ok

84b1f48

Include QRegularExpression

a5e2f17

Add \ingroup gui

01e428d

FetchMore API Override

24a9c1a

Doxy

7460e4f

Fix unreported bug in ORACLE provider

a3b9ced

Error in spatial index exists check.

Fix test (now geom col is exposed)

336a64d

lint waste my time

af26206

Address PR comments

78a42cf

elpaso force-pushed the connections-api-results-widget branch from 6d4e64c to 78a42cf Compare July 6, 2021 20:38

nyalldawson reviewed Jul 6, 2021

View reviewed changes

elpaso added 4 commits July 7, 2021 12:43

Address PR comments - part 2

878b243

Update oracle connection capabilities

9d5d43e

Label POSTGRES test

3cac2e7

Better logic for sql layer tests

a94e089

nyalldawson approved these changes Jul 9, 2021

View reviewed changes

elpaso merged commit 91370a9 into qgis:master Jul 10, 2021

elpaso deleted the connections-api-results-widget branch July 10, 2021 13:30

elpaso mentioned this pull request Jul 16, 2021

Database connection utility #44200

Merged

stefanuhrig reviewed Aug 4, 2021

View reviewed changes

zacharlie added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Sep 19, 2021

zacharlie added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Sep 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connections api results widget #44051

Connections api results widget #44051

elpaso commented Jul 5, 2021

nyalldawson commented Jul 5, 2021

elpaso commented Jul 6, 2021

nyalldawson commented Jul 6, 2021

elpaso commented Jul 7, 2021

nyalldawson commented Jul 7, 2021

nyalldawson commented Jul 11, 2021

elpaso commented Jul 11, 2021

stefanuhrig Aug 4, 2021

mrylov Aug 4, 2021

sigeal commented Mar 24, 2022 •

edited

Loading

Connections api results widget #44051

Connections api results widget #44051

Conversation

elpaso commented Jul 5, 2021

Description

Features

nyalldawson commented Jul 5, 2021

elpaso commented Jul 6, 2021

nyalldawson commented Jul 6, 2021

elpaso commented Jul 7, 2021

nyalldawson commented Jul 7, 2021

nyalldawson commented Jul 11, 2021

elpaso commented Jul 11, 2021

stefanuhrig Aug 4, 2021

Choose a reason for hiding this comment

mrylov Aug 4, 2021

Choose a reason for hiding this comment

sigeal commented Mar 24, 2022 • edited Loading

sigeal commented Mar 24, 2022 •

edited

Loading