Query Search Page #1122

vera-liu · 2016-09-16T00:04:30Z

Query search page under SQL Lab in the menu header

What's done:

Endpoint for getting all users
Endpoint for retrieving all queries with a databaseId/userId/status/string for filtering on sql
Query Search page with four fields
Made modifications based on comments, ready for second round review: @mistercrunch
Todo:
Add visualize modal triggered in action bar to QuerySearchTable as that in QueryHistory
make userId and dbId in Query result clickable links to set a specific userId or dbId in filter
Make a copy-url function beside each entry of query result, once generated the url will lead user to a specific query
Time range for searching queries
Alerts or Loading icon while queries are being fetched

bkyryliuk · 2016-09-16T01:11:51Z

caravel/assets/javascripts/SqlLab/actions.js

@@ -17,6 +17,8 @@ export const QUERY_EDITOR_SET_TITLE = 'QUERY_EDITOR_SET_TITLE';
 export const QUERY_EDITOR_SET_AUTORUN = 'QUERY_EDITOR_SET_AUTORUN';
 export const QUERY_EDITOR_SET_SQL = 'QUERY_EDITOR_SET_SQL';
 export const SET_DATABASES = 'SET_DATABASES';
+export const SET_FILTER_DATABASES = 'SET_FILTER_DATABASES';


prefix them QUERY_SEARCH

bkyryliuk · 2016-09-16T01:15:16Z

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

+  componentWillReceiveProps(nextProps) {
+    this.refreshQueries(nextProps);
+  }
+  getQueryState(val) {


getQueryState(val) {
switch (val) {
case 1:
return 'success';
etc will be a bit less of code

bkyryliuk · 2016-09-16T01:16:00Z

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

+  refreshQueries(nextProps) {
+    const userId = nextProps.queryFilter.userId;
+    const dbId = nextProps.queryFilter.queryDbId;
+    let sql = nextProps.queryFilter.searchText;


s/sql/searchText ?

bkyryliuk · 2016-09-16T01:17:10Z

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

+          const q = data[id];
+          newQueriesArray.push(q);
+        }
+        this.setState({ queriesArray: newQueriesArray });


could you just do this.setState({ queriesArray: data }); ?

data is an object, I had to transfer it to an array for QueryTable, not sure if there's a better option

bkyryliuk · 2016-09-16T01:22:21Z

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

+  constructor(props) {
+    super(props);
+    this.state = {
+      databaseLoading: false,


you could just check if the list of the databases is empty or not rather than keeping it in separate var.

bkyryliuk · 2016-09-16T01:25:03Z

caravel/views.py

+            mimetype="application/json")
+
+    @has_access
+    @expose("/search_queries/userId=<userId>"


maybe s/search_queries/sql_lab/search

bkyryliuk · 2016-09-16T01:26:09Z

caravel/views.py

+
+    @has_access
+    @expose("/search_queries/userId=<userId>"
+        "&dbId=<dbId>&sql=<search_text>&state=<state>")


you may want to make want to make the request post or somehow handle special characters

bkyryliuk · 2016-09-16T01:26:38Z

caravel/views.py

+        """Get all the queries."""
+        if not g.user.get_id():
+            return Response(
+                json.dumps({'error': "Please login to access the queries."}),


update error message

bkyryliuk · 2016-09-16T01:28:02Z

caravel/views.py

+        search_str = '%{}%'.format(search_text)
+
+        query = db.session.query(models.Query)
+        if not userId == 'null':


I believe sqlalchemy is fine with the None values, it just ignores them, your query could look like:
query = query.filter(
models.Query.user_id == userId,
models.Query.database_id == dbId, ...
@mistercrunch - could you confirm that?

bkyryliuk · 2016-09-16T01:28:38Z

caravel/views.py

+        search_str = '%{}%'.format(search_text)
+
+        query = db.session.query(models.Query)
+        if not userId == 'null':


userId = None if userId == 'null' else userId

bkyryliuk · 2016-09-16T01:29:02Z

caravel/views.py

+        if not search_text == 'null':
+            query = query.filter(models.Query.sql.like(search_str))
+
+        sql_queries = query.limit(1000).all()


make it a configurable param in the config.py

bkyryliuk · 2016-09-16T01:30:19Z

caravel/views.py


+appbuilder.add_link('My Queries',


s/My Queries/SQL Editor ?

mistercrunch · 2016-09-16T04:05:58Z

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

+  }
+  getQueryState(val) {
+    let status;
+    switch (val) {


no need to map strings to arbitrary ints, let's just use the strings directly

mistercrunch · 2016-09-16T04:13:15Z

caravel/views.py

+    @log_this
+    def search_queries(self, userId, dbId, search_text, state):
+        """Get all the queries."""
+        if not g.user.get_id():


@has_access should take care of that

mistercrunch · 2016-09-16T04:15:16Z

caravel/views.py

+            # Filter on user Id
+            query = query.filter(models.Query.user_id == userId)
+
+        if not dbId == 'null':


that's oddly twisted, why no if dbId != 'null':

if not (not (not True)):

mistercrunch · 2016-09-16T04:21:07Z

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

@@ -0,0 +1,76 @@
+import React from 'react';


This component is largely similar (if not directly copied) from the existing QueryTable component. Why not just use that component? It receives an array of queries and a list of columns to expose which should work perfectly.

mistercrunch · 2016-09-16T04:21:59Z

caravel/assets/javascripts/SqlLab/index.jsx

@@ -27,9 +28,20 @@ if (process.env.NODE_ENV === 'dev') {
 let store = createStore(sqlLabReducer, initialState, enhancer);

 // jquery hack to highlight the navbar menu
-$('a[href="/caravel/sqllab"]').parent().addClass('active');
+// $('a[href="/caravel/sqllab"]').parent().addClass('active');


why is this not needed anymore?

This always highlights the 'SQL Lab/My Queries' in the dropdown menu when 'Query Search' is selected. Not sure how to work around this

mistercrunch · 2016-09-16T04:43:40Z

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

+    let sql = nextProps.queryFilter.searchText;
+    if (sql === '') sql = 'null';
+    const showState = this.getQueryState(nextProps.queryFilter.queryState);
+    const url = `


is this a line break after the backtick? my brain interpreter doesn't understand how this work...

mistercrunch · 2016-09-16T04:46:41Z

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

+      const options = data.result.map((db) => ({ value: db.id, label: db.database_name }));
+      this.props.actions.setFilterDatabases(data.result);
+      this.setState({ databaseOptions: options });
+      this.setState({ databaseLoading: false });


you can call setState only once here instead of twice

mistercrunch · 2016-09-16T04:46:56Z

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

+      if (status === 'success') {
+        const options = data.map((user) => ({ value: user[0], label: user[1] }));
+        this.setState({ userLoading: false });
+        this.setState({ userOptions: options });


single call to setState

mistercrunch · 2016-09-16T04:47:19Z

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

+        this.setState({ userLoading: false });
+        this.setState({ userOptions: options });
+      } else {
+        console.log('get users failed');


mistercrunch · 2016-09-16T04:49:32Z

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

+    const val = (status) ? status.value : null;
+    this.setState({ queryState: val });
+  }
+  fetchDatabaseOptions() {


this is copy pasta from SqlEditorLeft, we should refactor a new component DatabaseSelect out and use it in both places

Once we get DatabaseSelect as a child component, the select value would have to be passed from it back to QuerySearch or QueryEditor. Do we want to pass the value from child to parent?

vera-liu · 2016-09-19T17:11:24Z

caravel/views.py

+href='/caravel/sqllab', icon="fa-flask", category='SQL Lab')
+appbuilder.add_link('Query Search',
+
+href='/caravel/sqllab?search', icon="fa-flask", category='SQL Lab')


I tried to use hashtag here, but if I use sqllab#search,to switch from SQL Editor to Query Search, the user has to press 'refresh', not sure if there's a better appraoch

you have to bind an even to hash change:
https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onhashchange

mistercrunch

A few minor comments, but overall looks like this is almost done!

mistercrunch · 2016-09-20T19:28:43Z

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

+}
+
+DatabaseSelect.propTypes = {
+  onDatabaseSelected: React.PropTypes.func,


I'd this with the standard name: onChange

mistercrunch · 2016-09-20T19:31:10Z

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

+    for (let i = 0; i < params.length; i++) {
+      if (i === params.length - 1) url += params[i];
+      else url += params[i] + '&';
+    }


this whole function can be replaced by:
return baseUrl + '?' + params.join('&');

mistercrunch · 2016-09-20T19:36:55Z

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

+        const newQueriesArray = [];
+        for (const id in data) {
+          const q = data[id];
+          newQueriesArray.push(q);


one line instead of 2:

newQueriesArray.push(data[id]);

mistercrunch · 2016-09-20T19:37:38Z

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

          </div>
+          <div className="col-sm-2">
+            <DatabaseSelect onDatabaseSelected={this.onDatabaseSelected.bind(this)} />


related to comment above: onChange={this.onDatabaseSelected.bind(this)

mistercrunch · 2016-09-20T19:39:13Z

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

@@ -122,7 +122,7 @@ QueryTable.propTypes = {
  queries: React.PropTypes.array,
 };
 QueryTable.defaultProps = {
-  columns: ['state', 'started', 'duration', 'progress', 'rows', 'sql', 'actions'],
+  columns: ['state', 'progress', 'sql'],


why not these fields: started, duration & rows?

mistercrunch · 2016-09-20T19:40:41Z

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

-            autosize={false}
-            onChange={this.changeDb.bind(this)}
-          />
+          <DatabaseSelect onDatabaseSelected={this.onDatabaseSelected.bind(this)} />


ascott · 2016-09-20T20:08:18Z

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

+    if (!(db)) {
+      this.setState({ tableOptions: [] });
+      return;
+    }


could this be an else statement, rather than an early return?

if (!db) { // set state } else { // do other things }

ascott · 2016-09-20T20:31:32Z

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

+  fetchDatabaseOptions() {
+    this.setState({ databaseLoading: true });
+    const url = '/databaseasync/api/read?_flt_0_expose_in_sqllab=1';
+    $.get(url, (data) => {


i think we should use a smaller lib for ajax/get/post requests. there is a polyfill for the window.fetch standard.
https://www.npmjs.com/package/whatwg-fetch

it's only ~3kb gzipped compared to ~70kb to include jquery on the page.

ascott · 2016-09-20T20:32:12Z

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

+  fetchDatabaseOptions() {
+    this.setState({ databaseLoading: true });
+    const url = '/databaseasync/api/read?_flt_0_expose_in_sqllab=1';
+    $.get(url, (data) => {


not a blocker for this PR. i can create a new issue to go through and change how we do this across the app.

vera-liu · 2016-09-20T23:59:01Z

Added specs and endpoint test, made more modifications
@mistercrunch @ascott @bkyryliuk

mistercrunch · 2016-09-23T19:13:29Z

LGTM

based on second round comments

…5/files

vera-liu force-pushed the vliu_query_search branch from 815f9d4 to 132dd08 Compare September 16, 2016 00:05

vera-liu mentioned this pull request Sep 16, 2016

Query Search - All Queries, My Saved Queries, My Recent Queries #857

Closed

vera-liu force-pushed the vliu_query_search branch from 132dd08 to c532b49 Compare September 16, 2016 01:05

bkyryliuk reviewed Sep 16, 2016

View reviewed changes

mistercrunch requested changes Sep 16, 2016

View reviewed changes

vera-liu commented Sep 19, 2016

View reviewed changes

vera-liu force-pushed the vliu_query_search branch 4 times, most recently from 148f798 to 839733a Compare September 19, 2016 20:49

mistercrunch requested changes Sep 20, 2016

View reviewed changes

ascott reviewed Sep 20, 2016

View reviewed changes

vera-liu force-pushed the vliu_query_search branch from a93d374 to 235d0a9 Compare September 20, 2016 23:57

vera-liu force-pushed the vliu_query_search branch from 235d0a9 to 85cbd83 Compare September 23, 2016 19:29

vera-liu added 5 commits September 23, 2016 13:57

Query search page under SQL Lab tab

56ff9f2

Modifications based on comments

d606dda

Hash

94ebb38

Added spec and endpoint test with modifications

f5abc5b

based on second round comments

Changed permission menu in https://github.com/airbnb/caravel/pull/109…

821f676

…5/files

vera-liu force-pushed the vliu_query_search branch from fc120a5 to 821f676 Compare September 23, 2016 20:57

vera-liu added 2 commits September 23, 2016 14:38

Merge branch 'master' into vliu_query_search

fda0174

Merge branch 'master' into vliu_query_search

1b7f8cd

vera-liu merged commit d5c5c0d into apache:master Sep 23, 2016

vera-liu mentioned this pull request Sep 23, 2016

Revert "Query Search Page" #1186

Merged

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

feat: add orderby shared control (apache#1122)

d9328f4

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

feat: add orderby shared control (apache#1122)

a312760

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

feat: add orderby shared control (apache#1122)

d996312

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

feat: add orderby shared control (apache#1122)

6675e60

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


		appbuilder.add_link('My Queries',

Query Search Page #1122

Query Search Page #1122

Conversation

vera-liu commented Sep 16, 2016 • edited Loading

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

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 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

vera-liu commented Sep 20, 2016 • edited Loading

mistercrunch commented Sep 23, 2016

vera-liu commented Sep 16, 2016 •

edited

Loading

vera-liu commented Sep 20, 2016 •

edited

Loading