[sqllab] add support for results backends #1377

mistercrunch · 2016-10-19T20:57:11Z

@ascott @vera-liu @bkyryliuk
Long running SQL queries (beyond the scope of a web request) can now use
a k/v store to hold their result sets.

Side missions:

Use redux-thunk for async query processing
Added a data preview option for queries that ran async
Data preview for any table element in the left panel
Make a bunch of components more flexible to satisfy the use-cases here

bkyryliuk

Great velocity !
I've left couple comments.

bkyryliuk · 2016-10-19T23:23:42Z

caravel/assets/javascripts/SqlLab/actions.js

+      dataType: 'json',
+      url: sqlJsonUrl,
+      success(results) {
+        // console.log(results);


delete this

bkyryliuk · 2016-10-19T23:23:54Z

caravel/assets/javascripts/SqlLab/actions.js

+        dispatch(querySuccess(query, results));
+      },
+      error() {
+        // console.log(err);


bkyryliuk · 2016-10-19T23:25:37Z

caravel/assets/javascripts/SqlLab/actions.js

+      sql: query.sql,
+      sql_editor_id: query.sqlEditorId,
+      tab: query.tab,
+      // tmp_table_name: this.state.ctas,


add a todo to fix it ?

oh right! I never finished that part

bkyryliuk · 2016-10-19T23:26:55Z

caravel/assets/javascripts/SqlLab/actions.js

+        // console.log(results);
+        dispatch(querySuccess(query, results));
+      },
+      error() {


s/error/error(err, textStatus, errorThrown) ?

I think it's ok to just notify of the failure without the underlying details

bkyryliuk · 2016-10-19T23:27:37Z

caravel/assets/javascripts/SqlLab/actions.js

+        }
+      },
+      error(err, textStatus, errorThrown) {
+        let msg;


it would be great to have this method in the utils. optional.

yeah I had the same feeling that we should generalize that somehow, we're supposed to start using fetch instead of jquery, maybe then would be a good time

bkyryliuk · 2016-10-20T00:00:01Z

caravel/views.py

@@ -1964,6 +1964,38 @@ def cached_key(self, key):
        return "nope"

    @has_access_api
+    @expose("/results/<key>/")
+    @log_this
+    def results(self, key):


if we'll have key as a query_name + timestamp we would be able to tell the user if the result set was expired

bkyryliuk · 2016-10-20T00:01:26Z

caravel/views.py

+            obj = json.loads(json_payload)
+            db_id = obj['query']['dbId']
+            session = db.session()
+            mydb = session.query(models.Database).filter_by(id=db_id).first()


s/first()/one()
it will fail if the DB was removed and will provide meaningful error message

bkyryliuk · 2016-10-20T00:02:36Z

caravel/views.py

+
+            return Response(
+                json_payload,
+                status=202,


s/202/200 ?
processing completed at this stage

bkyryliuk · 2016-10-20T00:03:35Z

caravel/views.py

+                        "re-run the query."
+                    )
+                }),
+                status=202,


s/202/410 ?
The requested resource is no longer available at the server and no forwarding address is known.

bkyryliuk · 2016-10-20T00:04:37Z

caravel/views.py

+                mimetype="application/json")
+        else:
+            return Response(
+                json.dumps({


s/json_dumps/json_error_response("Data could not be retrived. You may want to re-run the query", status=410)

mistercrunch · 2016-10-20T05:11:37Z

Thanks for the thorough review, I think I addressed and/or commented on all the comments.

mistercrunch · 2016-10-20T21:47:41Z

caravel/assets/javascripts/SqlLab/components/DataPreviewModal.jsx

+import ResultSet from './ResultSet';
+
+const propTypes = {
+  queries: React.PropTypes.array,


bkyryliuk · 2016-10-20T22:27:01Z

🚢

Long running SQL queries (beyond the scope of a web request) can now use a k/v store to hold their result sets.

* [sqllab] add support for results backends Long running SQL queries (beyond the scope of a web request) can now use a k/v store to hold their result sets. * Addressing comments, fixed js tests * Fixing mysql has gone away * Adressing more comments * Touchups

vera-liu · 2016-10-25T00:45:15Z

caravel/assets/javascripts/SqlLab/components/App.jsx

@@ -37,16 +39,18 @@ class App extends React.Component {
        </div>
      );
    }
+    content = (


I realized content here gets reassigned when this.state.hash is true, so QuerySearch page is not shown, is this intended? @mistercrunch

bkyryliuk requested changes Oct 20, 2016

View reviewed changes

bkyryliuk approved these changes Oct 20, 2016

View reviewed changes

mistercrunch commented Oct 20, 2016

View reviewed changes

mistercrunch added 4 commits October 20, 2016 16:54

[sqllab] add support for results backends

d86abc8

Long running SQL queries (beyond the scope of a web request) can now use a k/v store to hold their result sets.

Addressing comments, fixed js tests

fa7f16f

Fixing mysql has gone away

5e2d08b

Adressing more comments

6d51dc5

mistercrunch force-pushed the results_backends branch from f9d73dc to 6d51dc5 Compare October 20, 2016 23:59

Touchups

7edfaa5

mistercrunch force-pushed the results_backends branch from 224b9b9 to 7edfaa5 Compare October 21, 2016 06:23

mistercrunch merged commit 6fb3b30 into apache:master Oct 21, 2016

mistercrunch deleted the results_backends branch October 21, 2016 06:40

vera-liu reviewed Oct 25, 2016

View reviewed changes

dpgaspar mentioned this pull request Jun 2, 2020

feat(api): bump marshmallow and FAB to version 3 #9964

Merged

6 tasks

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021

add y-axis control to bubble chart (apache#1377)

e175aff

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021

add y-axis control to bubble chart (apache#1377)

497a1f0

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021

add y-axis control to bubble chart (apache#1377)

bc0c85b

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021

add y-axis control to bubble chart (apache#1377)

de86eb9

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.12.0 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sqllab] add support for results backends #1377

[sqllab] add support for results backends #1377

mistercrunch commented Oct 19, 2016

bkyryliuk left a comment

bkyryliuk Oct 19, 2016

bkyryliuk Oct 19, 2016

bkyryliuk Oct 19, 2016

mistercrunch Oct 20, 2016

bkyryliuk Oct 19, 2016

mistercrunch Oct 20, 2016

bkyryliuk Oct 19, 2016

mistercrunch Oct 20, 2016

bkyryliuk Oct 20, 2016

bkyryliuk Oct 20, 2016

bkyryliuk Oct 20, 2016

bkyryliuk Oct 20, 2016

bkyryliuk Oct 20, 2016

mistercrunch commented Oct 20, 2016

mistercrunch Oct 20, 2016

bkyryliuk commented Oct 20, 2016

vera-liu Oct 25, 2016

[sqllab] add support for results backends #1377

[sqllab] add support for results backends #1377

Conversation

mistercrunch commented Oct 19, 2016

bkyryliuk 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

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

mistercrunch commented Oct 20, 2016

Choose a reason for hiding this comment

bkyryliuk commented Oct 20, 2016

Choose a reason for hiding this comment