[FIX] OWSql does not save selected table/query #2659

ceprio · 2017-10-08T01:39:10Z

Issue

Added functionality to the widget:

Description of changes

The widget now saves the settings of the selected table name and SQL Query
Restoring of table name and SQL query is also now working when opening a workflow
Connection to the database is done earlier to allow the tables list to be fetched
Table name is saved and restored as text (or name of table) instead of Id (as previously)
Empty username and password are now saved as '' (empty string) instead of None. This allows the use of SQL Server's Windows authentication (use of the current user credentials if both fields are empty).
Placing 'Custom SQL' in second in the table list in order to see it faster when the list of tables is long.
Execute SQL query only if minimum number of chars entered
Note also:
A bug was found within SQL Server 2012 (and earlier versions it seem) that had to be dealt with in the backend mssql.py. I added the fix in this pull request so the widget can be tested correctly even on this SQL server version. More details in the source code.

Includes

Code changes
Tests
Documentation

Added functionality to the widget: - The widget now saves the settings of the selected table name and SQL Query - Restoring of table name and SQL query is also now working when opening a workflow - Connection to the database is done earlier to allow the tables list to be fetched - Table name is saved and restored as text (or name of table) instead of Id (as previously) - Empty username and password are now saved as '' (empty string) instead of None. This allows the use of SQL Server's Windows authentication (use of the current user credentials if both fields are empty). - Placing 'Custom SQL' in second in the table list in order to see it faster when the list of tables is long. - Execute SQL query only if minimum number of chars entered

CLAassistant · 2017-10-08T01:39:16Z

All committers have signed the CLA.

astaric

Thank you for the pull request.

I have left some comments, but they are mostly related to code style/naming, the features you have implemented look ok. Let me know when you address the comments or if you have any questions.

astaric · 2017-10-10T11:11:28Z

Orange/widgets/data/owsql.py

@@ -1,3 +1,5 @@
+#!/usr/bin/python


As widgets are mostly used from Canvas, we usually do not include this. For testing, widget can be run with python widget_module.py.

astaric · 2017-10-10T11:14:29Z

Orange/widgets/data/owsql.py

    def data(self, index, role=Qt.DisplayRole):
        row = index.row()
        if role == Qt.DisplayRole:
            return str(self[row])
        return super().data(index, role)


-class BackendModel(PyListModel):
+class BackendModels(PyListModel):


Model is a part of the Qt's Model/View paradigm and should remain singular.

I was surprised to see that the Model was is fact a list of backends and you have to select one. Thus the 's'. I'll correct.

astaric · 2017-10-10T11:15:31Z

Orange/widgets/data/owsql.py

@@ -85,10 +87,10 @@ def __init__(self):
        vbox = gui.vBox(self.controlArea, "Server", addSpace=True)
        box = gui.vBox(vbox)

-        self.backendmodel = BackendModel(Backend.available_backends())
+        self.backendmodels = BackendModels(Backend.available_backends())


Same as above, model should remain singular. If you prefer plural, I would simply use
self.backends = BackendModel(...)

Oh, ok I'll do that.

astaric · 2017-10-10T11:17:05Z

Orange/widgets/data/owsql.py

+        index = self.tablecombo.findText(str(self.table))
+        if index != -1:
+            self.tablecombo.setCurrentIndex(index)
+        self.tablecombo.activated[int].connect(self.select_table) # set up the callback to select_table in case of selection change


Could you move the commend in a line before the command? We try to limit the lines in source code to maximum of 100 characters (though some go even further and limit it to 79)

astaric · 2017-10-10T11:18:11Z

Orange/widgets/data/owsql.py


    def select_table(self):
+        "Called on tablecombo selection change"


""" is usually used for docstrings

astaric · 2017-10-10T11:18:38Z

Orange/widgets/data/owsql.py

@@ -347,6 +360,7 @@ def get_table(self):
                domain = s.get_domain(inspect_values=True)
                self.Information.data_sampled()
            else:
+#


Please remove

…r 2012

codecov-io · 2017-10-11T19:41:48Z

Codecov Report

Merging #2659 into master will increase coverage by 0.34%.
The diff coverage is 2.63%.

@@            Coverage Diff            @@
##           master   #2659      +/-   ##
=========================================
+ Coverage   75.46%   75.8%   +0.34%     
=========================================
  Files         331     338       +7     
  Lines       57967   59469    +1502     
=========================================
+ Hits        43743   45082    +1339     
- Misses      14224   14387     +163

astaric · 2017-10-12T07:52:24Z

Orange/widgets/data/owsql.py

@@ -20,7 +21,7 @@
 MAX_DL_LIMIT = 1000000


-class TableModel(PyListModel):
+class TableModels(PyListModel):


This one should be singular Model as well

Should have anticipated this one.

astaric · 2017-10-12T07:52:32Z

Orange/widgets/data/owsql.py

@@ -124,17 +125,24 @@ def __init__(self):
        box.layout().addWidget(self.passwordtext)

        self._load_credentials()
+        self.tablemodels = TableModels()


tablemodel or tables, but not tablemodels

astaric · 2017-10-12T07:53:29Z

Orange/widgets/data/owsql.py

@@ -347,6 +360,7 @@ def get_table(self):
                domain = s.get_domain(inspect_values=True)
                self.Information.data_sampled()
            else:
+


I do not think this empty line is necessary

astaric · 2017-10-12T08:05:01Z

Orange/data/sql/backend/mssql.py

                cur.execute("SET SHOWPLAN_XML ON")
                try:
                    cur.execute(query)
                    result = cur.fetchone()
                    return int(self.EST_ROWS_RE.search(result[0]).group(1))
+                except AttributeError:
+                # This is to catch a bug from SQL Server 2012


If I understand correctly, the query plan in SQL Server 2012 produces an XML, which does not match the EST_ROWS_RE regexp as it represents the number of rows as float? If this is the case, why not update the regular expression to match both?

Instead of

StatementEstRows="(\d+)"

which expects an integer between the parentheses, we could use something like this

r'StatementEstRows="(\d+)(\.\d*)?"'

the second regular expressions allow the number to end with a decimal point and some numbers, but only parses the whole part

Not exacty, after searching it seem that our SQL server statistics are not maintained often enough and this results in bad values being returned in form of a float thus the AttributeError. I'm not sure if a well maintained server would return a float: I have a second database where statistics are maintained and I never received a float from the Plan query.
It would require more samples to figure out what to do. This code works in all situations, the drawback is the execution time if a float is returned. Maybe there should be a warning issued like those deprecated warnings found in some libraries saying that "statistics are out of date, SELECT count(*) used." but I do not know how to throw such a message in Orange.

I modified the code comments to explain the reason of the AttributeError. If someone ever comes back with a float with the right estimated result, the code will have to be updated as you suggested and if possible with a sanity check on the statistics (so un-maintained statistics would be caught).

ceprio · 2017-10-14T17:15:22Z

The code has been updated, please check comments (especially mssql.py).
Thx

astaric · 2017-10-16T08:15:56Z

Orange/data/sql/backend/mssql.py

+                except AttributeError:
+                # This is to catch a float received in StatementEstRows
+                # a float is received when the server's statistics are out of date.
+                    pass


I have read your comment and I agree, if the float is a result of a reallly bad estimate, it should not be used. I still do not like the flow of this function though. I would put it like this:

match = self.EST_ROWS_RE.search(result[0]) if not match: # Comment explaining why this happens return None return int(match.group(1))

astaric · 2017-10-16T08:17:47Z

Orange/data/sql/backend/mssql.py

+                raise BackendError(str(ex)) from ex
+            # In case of an AttributeError, give a second chance:
+            # Use the long method for counting
+            cur.execute("SELECT count(*) FROM ( {} ) x".format(query))


This should not be necessary. When count_approx returns None, an exact count is already performed (see SqlTable.count_approx)

ceprio · 2017-10-16T20:54:23Z

Your proposal makes much sense, I had not seen SqlTable.count_approx. Thank you for that.
Tested with success and your way also catches the non-existance of StatementEstRows in the result if that ever appends.
Thank you,

Eclipse does not seem to be able to take care of these...

astaric · 2017-10-17T07:01:19Z

Thank you for your contribution. It will be included in the next release (planned for the last week of October)

astaric reviewed Oct 10, 2017

View reviewed changes

Changed file according to reviewer comments and bug fix for SQL Serve…

8ae9a1a

…r 2012

Correction following comments from Travis CI

a1e6411

astaric reviewed Oct 12, 2017

View reviewed changes

Cosmetic changes following reviewer's comment

8ba7584

Removed white space

34433be

astaric reviewed Oct 16, 2017

View reviewed changes

Code change to allow exact count be made by the callee.

f791857

Removed spaces at end of lines

6e702a0

Eclipse does not seem to be able to take care of these...

astaric changed the title ~~Update owsql.py~~ [FIX] OWSql does not save selected table/query Oct 17, 2017

astaric merged commit 37616b9 into biolab:master Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] OWSql does not save selected table/query #2659

[FIX] OWSql does not save selected table/query #2659

ceprio commented Oct 8, 2017 •

edited

Loading

CLAassistant commented Oct 8, 2017 •

edited

Loading

astaric left a comment

astaric Oct 10, 2017

astaric Oct 10, 2017

ceprio Oct 11, 2017

astaric Oct 10, 2017

ceprio Oct 11, 2017

astaric Oct 10, 2017

astaric Oct 10, 2017

astaric Oct 10, 2017

codecov-io commented Oct 11, 2017 •

edited

Loading

astaric Oct 12, 2017

ceprio Oct 14, 2017

astaric Oct 12, 2017

astaric Oct 12, 2017

astaric Oct 12, 2017

ceprio Oct 14, 2017

ceprio Oct 14, 2017

ceprio commented Oct 14, 2017

astaric Oct 16, 2017

astaric Oct 16, 2017

ceprio commented Oct 16, 2017

astaric commented Oct 17, 2017


		def select_table(self):
		"Called on tablecombo selection change"

[FIX] OWSql does not save selected table/query #2659

[FIX] OWSql does not save selected table/query #2659

Conversation

ceprio commented Oct 8, 2017 • edited Loading

Issue

Description of changes

Includes

CLAassistant commented Oct 8, 2017 • edited Loading

astaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 11, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceprio commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceprio commented Oct 16, 2017

astaric commented Oct 17, 2017

ceprio commented Oct 8, 2017 •

edited

Loading

CLAassistant commented Oct 8, 2017 •

edited

Loading

codecov-io commented Oct 11, 2017 •

edited

Loading