Added time filter to query search page #1329

vera-liu · 2016-10-12T18:08:20Z

Based on feedback from the meeting, added from-to time filter for query search

bkyryliuk · 2016-10-12T19:52:56Z

@vera-liu - could you also display query creation time in the search results ?

vera-liu · 2016-10-12T21:14:59Z

bkyryliuk · 2016-10-13T22:05:19Z

caravel/assets/javascripts/SqlLab/common.js

+  'running',
+];
+
+export const FROM_OPTIONS = [


could we reuse the options?

bkyryliuk · 2016-10-13T22:07:04Z

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

-  insertParams(baseUrl, params) {
-    return baseUrl + '?' + params.join('&');
+  getTimeFromSelection(selection) {
+    switch (selection) {


what about storing dictionary like:
{'1 hour ago': 1}
and then use: hoursAgo(dict['selection']) ?
now would be 0 hours ago and you could use 0 hours ago, days and years could be expressed in hours too

I think year might be tricky here since it's not guaranteed to be a fixed number of hours?

what about storing the dict from option to the function that evaluates it ?

I tried using the dict from value to function, but the function is evaluated when the dict is defined. There might be a way to delay the evaluation, but I figured switch statements might be a better option for mapping to functions?

function should take the attr, not to be evaluated.
@mistercrunch @ascott - what is your take here?

i think switch/case works fine in this case. i think the function names could be made more clear. what is the advantage of using an object mapping over case/switch here @bkyryliuk ?

@ascott - less boilerplate.

getTimeFromSelection(selection) { return this.function_obj[selection]() }

or smth like that

bkyryliuk · 2016-10-13T22:08:09Z

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

+          <div className="col-sm-1">
+            <Select
+              name="select-from"
+              placeholder="[From-]"


NIT, strings will have to be i18n eventually

Sorry I don't understand..could you elaborate more?

internationalization

could you give me an example?

we don't have any internationalization support on the front end yet. we'll have to do a pass on the entire front end since there are a lot of instances that are not wrapped yet.

bkyryliuk · 2016-10-13T22:10:06Z

caravel/assets/spec/javascripts/sqllab/QuerySearch_spec.jsx

@@ -15,7 +15,7 @@ describe('QuerySearch', () => {

  it('should have two Select', () => {
    const wrapper = shallow(<QuerySearch />);
-    expect(wrapper.find(Select)).to.have.length(2);
+    expect(wrapper.find(Select)).to.have.length(4);


would be great to test date functions.
Do you have any plans write tests with state mutations ?

hmm by state mutations do you mean tests for reducers? for now we haven't had much tests for functionalities, most are for rendering components in javascript

more like tests that test the logic rather than presence of the components.
it you select option expect that state is updated, etc
if you execute search - except url is like expected

bkyryliuk · 2016-10-13T22:11:34Z

caravel/views.py


-        if userId != 'null':
+        if user_id != 'null':


s/if user_id != 'null':/ if user_id

same for others

bkyryliuk · 2016-10-13T22:11:56Z

caravel/views.py

-        userId = request.args.get('userId')
-        databaseId = request.args.get('databaseId')
-        searchText = request.args.get('searchText')
+        user_id = request.args.get('userId')


s/user_id/search_user_id - not to confuse with the logged in user

bkyryliuk · 2016-10-13T22:14:18Z

caravel/views.py

+        if from_time != 'null':
+            query = query.filter(models.Query.start_time > int(from_time))
+
+        if to_time != 'null':


add unit test that show that to and from works

bkyryliuk · 2016-10-13T22:16:53Z

caravel/views.py

-        databaseId = request.args.get('databaseId')
-        searchText = request.args.get('searchText')
+        user_id = request.args.get('userId')
+        database_id = request.args.get('databaseId')


s/.get('databaseId')/.get('database_id') - to be consistent
for now we use python conversion for args

vera-liu · 2016-10-14T19:17:20Z

Added more specs and tests as required. Ready for final review @bkyryliuk

bkyryliuk · 2016-10-24T17:20:09Z

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

-      `searchText=${this.state.searchText}`,
-      `status=${this.state.status}`,
+      this.state.userId ?
+        `user_id=${this.state.userId}` : '',


it should fit into the same line

bkyryliuk · 2016-10-24T17:21:34Z

caravel/views.py

        status = request.args.get('status')
+        from_time = request.args.get('from')


please add a comment that time is the unix timestamp in seconds

bkyryliuk · 2016-10-24T17:22:12Z

caravel/views.py


+        sql_queries = query.limit(config.get("QUERY_SEARCH_LIMIT")).all()


QUERY_SEARCH_LIMIT may be not set or absent in the config. Please handle that case.

bkyryliuk · 2016-10-24T17:23:07Z

tests/core_tests.py

+        second_query = db.session.query(models.Query).filter_by(
+            sql='SELECT * FROM ab_permission').first()
+
+        from_time = 'from=' + \


it should fit into the same line.
Please use '{}'.format instead of + and str

bkyryliuk · 2016-10-24T17:27:07Z

tests/core_tests.py

+        resp = self.get_resp('/caravel/search_queries?'+'&'.join(params))
+        data = json.loads(resp)
+        self.assertEquals(1, len(data))
+        self.logout()


please test multiple use cases:
make sure that initially you have queries for 2 users, 1 user has queries for at least 2 databases

Test that:

time filter filters out queries both for to and from use cases

user_id filters given user queries

QUERY_SEARCH_LIMIT works

like query returns the expected queries

It would be nice to have all them as separate test case and you can you SetUp method to create initial dataset. Come by if you need some help with it.

ascott · 2016-10-24T17:40:20Z

caravel/assets/spec/javascripts/sqllab/DatabaseSelect_spec.jsx

+import sinon from 'sinon';
+
+describe('DatabaseSelect', () => {
+  it('should render', () => {


could break this up into 2 tests: it('is valid element') and it('renders'). it's a best practice to have an assertion for each test. so the render test should assert that some element rendered within DatabaseSelect

ascott · 2016-10-24T17:43:06Z

caravel/assets/spec/javascripts/sqllab/DatabaseSelect_spec.jsx

+    ).to.equal(true);
+  });
+
+  it('should call onChange', () => {


could be more explicit with this test name...it('calls onChange on select change')

ascott · 2016-10-24T17:44:52Z

caravel/assets/spec/javascripts/sqllab/QuerySearch_spec.jsx

+    expect(wrapper.find(Select)).to.have.length(4);
+  });
+
+  it('should update userId', () => {


i generally think using should in tests is not neccessary. could make this more clear: it('updates userId on user select change')

ascott · 2016-10-24T17:49:37Z

caravel/assets/javascripts/SqlLab/common.js

@@ -5,4 +5,18 @@ export const STATE_BSSTYLE_MAP = {
  success: 'success',
 };

-export const STATUS_OPTIONS = ['success', 'failed', 'running'];
+export const STATUS_OPTIONS = [


seems like this file should be called constants rather than common for clarity.

ascott · 2016-10-24T17:54:49Z

caravel/assets/javascripts/modules/dates.js

@@ -85,3 +85,24 @@ export const now = function () {
  return moment().utc().valueOf();
 };

+export const hoursAgo = function (h) {


could we use moment's Time from X methods here instead? http://momentjs.com/docs/#/displaying/from/

i misunderstood that these functions are returning seconds from epoch to x hours ago, so the time from X methods don't make sense here. i think we could rename these for clarity...

bkyryliuk · 2016-10-27T21:51:55Z

tests/core_tests.py

-        resp = self.client.get('/caravel/search_queries?'+'&'.join(params))
-        self.assertEquals(200, resp.status_code)
+        self.login('admin')
+        self.run_sql("SELECT * FROM ab_user", 'admin', client_id='client_id_1')


move query creation into the setUp function:
https://docs.python.org/2/library/unittest.html
also drop all queries before setting it up to avoid collision with other tests

make sre tearDown drops all queries too

We have run_sql() calls in a bunch of other test functions for different purposes, I'm afraid moving all of them to setUp() will result in rewriting all these functions

bkyryliuk · 2016-10-27T21:52:39Z

tests/core_tests.py

+        params = [from_time, to_time]
+        resp = self.get_resp('/caravel/search_queries?'+'&'.join(params))
+        data = json.loads(resp)
+        self.assertEquals(1, len(data))


assert that data has a query that falls into given period of time

bkyryliuk · 2016-10-27T21:53:46Z

tests/core_tests.py

+        # Test search queries on text
+        resp = self.get_resp('/caravel/search_queries?search_text=permission')
+        data = json.loads(resp)
+        self.assertEquals(1, len(data))


assert that result query text has permission in it

bkyryliuk · 2016-10-27T21:54:27Z

tests/core_tests.py

+        self.assertEquals(1, len(data))
+        resp = self.get_resp('/caravel/search_queries?database_id=2')
+        data = json.loads(resp)
+        self.assertEquals(0, len(data))


assert that there are no queries in the db that have db id = 2

bkyryliuk · 2016-10-27T21:54:38Z

tests/core_tests.py

+        # Test search queries on database Id
+        resp = self.get_resp('/caravel/search_queries?database_id=1')
+        data = json.loads(resp)
+        self.assertEquals(1, len(data))


assert that data has a query with db_id == 1

vera-liu · 2016-10-28T02:45:25Z

@bkyryliuk I updated the python tests, will move sqllab tests out of core_tests in a separate PR

bkyryliuk · 2016-10-28T16:16:36Z

tests/core_tests.py

+
+        self.login('gamma')
+        self.run_sql("SELECT * FROM ab_permission", 'gamma', client_id='client_id_2')
+        self.logout()

    def tearDown(self):
        pass


would be nice to drop queries you've created in the tear down

bkyryliuk · 2016-10-28T16:19:46Z

tests/core_tests.py

+      data = json.loads(resp)
+      self.assertEquals(3, len(data))
+      for k, v in data.items():
+        assert data[k]['dbId'] == 1


s/assert/self.assertEquals

better way would be

db_ids = [d[k]['dbId'] for k in data] self.assertEquals([1, 1, 1], db_ids)

bkyryliuk · 2016-10-28T16:23:23Z

tests/core_tests.py

+      self.logout()
+
+    def test_search_query_on_user(self):
+      self.login('admin')


you can move admin login and logout to the setup and tear down.
specific logins can be handled in the test function

but we have access tests/other login logout in other tests, wouldn't they cause conflicts if we log in at Setup()?

bkyryliuk · 2016-10-28T16:24:21Z

tests/core_tests.py

+      resp = self.get_resp('/caravel/search_queries?user_id={}'.format(user.id))
+      data = json.loads(resp)
+      self.assertEquals(2, len(data))
+      for k, v in data.items():


do the same approach that I suggested for the dbIds

bkyryliuk · 2016-10-28T16:24:59Z

tests/core_tests.py

+      resp = self.get_resp('/caravel/search_queries?user_id={}'.format(user.id))
+      data = json.loads(resp)
+      self.assertEquals(1, len(data))
+      for (k, v) in data.items():


ditto, not need to loop through as there is just 1 query

bkyryliuk · 2016-10-28T16:25:13Z

tests/core_tests.py

+      resp = self.get_resp('/caravel/search_queries?status=success')
+      data = json.loads(resp)
+      self.assertEquals(2, len(data))
+      for k, v in data.items():


bkyryliuk · 2016-10-28T16:26:03Z

tests/core_tests.py

+      resp = self.get_resp('/caravel/search_queries?search_text=permission')
+      data = json.loads(resp)
+      self.assertEquals(1, len(data))
+      for k, v in data.items():


by the way if you don't use the var you can name it _
for _, v in data.items()
better way would be I think to call data.iteritems or smth

bkyryliuk · 2016-10-28T16:27:32Z

tests/core_tests.py

+      data = json.loads(resp)
+      self.assertEquals(2, len(data))
+      for k, v in data.items():
+          assert v['startDttm'] > int(first_query_time) \


use assertLess,
when data = 2, you can actually check each item manually

vera-liu · 2016-10-28T16:54:53Z

all done @bkyryliuk

bkyryliuk · 2016-10-28T17:00:42Z

tests/core_tests.py

+      resp = self.get_resp('/caravel/search_queries?search_text=permission')
+      data = json.loads(resp)
+      self.assertEquals(1, len(data))
+      assert 'permission' in data.values()[0]['sql']


self.assertIn(a, b) :)

bkyryliuk · 2016-10-28T17:04:15Z

Lovely, thanks for bearing with me !
LGTM once the last comment is addressed.

vera-liu force-pushed the vliu_time_filter branch from f4c9837 to 912ef21 Compare October 12, 2016 18:20

vera-liu force-pushed the vliu_time_filter branch 4 times, most recently from cdb54c0 to b044d71 Compare October 12, 2016 21:12

vera-liu force-pushed the vliu_time_filter branch from 1265b43 to 6fdba82 Compare October 13, 2016 00:31

bkyryliuk reviewed Oct 13, 2016

View reviewed changes

vera-liu force-pushed the vliu_time_filter branch 2 times, most recently from 979049f to e7c589c Compare October 14, 2016 19:16

vera-liu mentioned this pull request Oct 14, 2016

Time Filter for query search #1327

Closed

bkyryliuk reviewed Oct 24, 2016

View reviewed changes

ascott reviewed Oct 24, 2016

View reviewed changes

bkyryliuk reviewed Oct 27, 2016

View reviewed changes

vera-liu added 2 commits October 27, 2016 18:54

Break python tests for separate functions, Move sql queries to setUp()

452fcb7

Get around eslint error for spec

3fc7d99

bkyryliuk reviewed Oct 28, 2016

View reviewed changes

vera-liu force-pushed the vliu_time_filter branch from eb65881 to 8ae7b16 Compare October 28, 2016 16:53

bkyryliuk reviewed Oct 28, 2016

View reviewed changes

vera-liu force-pushed the vliu_time_filter branch 3 times, most recently from 208e30b to bc052cc Compare October 28, 2016 18:26

Small changes based on comments

fa6de67

vera-liu force-pushed the vliu_time_filter branch from 3fe945f to fa6de67 Compare October 28, 2016 20:22

Merge branch 'master' into vliu_time_filter

79dc772

vera-liu merged commit 45efcb3 into apache:master Oct 28, 2016

vera-liu deleted the vliu_time_filter branch November 1, 2016 19:01

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

		status = request.args.get('status')
		from_time = request.args.get('from')


		sql_queries = query.limit(config.get("QUERY_SEARCH_LIMIT")).all()

Added time filter to query search page #1329

Added time filter to query search page #1329

Conversation

vera-liu commented Oct 12, 2016 • edited Loading

bkyryliuk commented Oct 12, 2016

vera-liu commented Oct 12, 2016

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

bkyryliuk Oct 13, 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

vera-liu Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vera-liu commented Oct 14, 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

ascott Oct 24, 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

bkyryliuk Oct 27, 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

vera-liu commented Oct 28, 2016

Choose a reason for hiding this comment

bkyryliuk Oct 28, 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

bkyryliuk Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

bkyryliuk Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

vera-liu commented Oct 28, 2016

Choose a reason for hiding this comment

bkyryliuk commented Oct 28, 2016

vera-liu commented Oct 12, 2016 •

edited

Loading

bkyryliuk Oct 13, 2016 •

edited

Loading

vera-liu Oct 13, 2016 •

edited

Loading

vera-liu commented Oct 14, 2016 •

edited

Loading

ascott Oct 24, 2016 •

edited

Loading

bkyryliuk Oct 27, 2016 •

edited

Loading

bkyryliuk Oct 28, 2016 •

edited

Loading

bkyryliuk Oct 28, 2016 •

edited

Loading

bkyryliuk Oct 28, 2016 •

edited

Loading