-
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api.py (Diff revision 1) Please see for moving all the filtering to the ORM (that way the work gets pushed down at the DB level instead of the API level)
e.g. https://github.com/cloudera/hue/blob/master/desktop/core/src/desktop/api.py
-
HUE-9492 [query browser] Text search, date filtering and pagination and unittest
Review Request #15493 — Created Oct. 6, 2020 and submitted
Information | |
---|---|
ayush.goyal | |
hue | |
master | |
HUE-9492 | |
Reviewers | |
hue | |
Amlesh1902, johan, ranade, romain, Sreenath, yingc |
commit e036b7220439a55fc88a37fd52aaabf4708fb6de (HEAD -> HUE-9492)
Author: ayush.goyal ayush.goyal@cloudera.com
Date: Thu Oct 8 16:15:56 2020 +0530HUE-9492 [query browser] adding unittest for Api query listing, pagination, query_countcommit 312d93fc5e6c02c9608aa5b6ed264e0a1e750d31
Author: ayush.goyal ayush.goyal@cloudera.com
Date: Thu Oct 8 01:56:21 2020 +0530HUE-9492 [query browser] Text search, date filtering and pagination
- 10
- 0
- 0
- 0
- 10
Description | From | Last Updated |
---|---|---|
Please see for moving all the filtering to the ORM (that way the work gets pushed down at the DB ... |
|
|
No paranthesis needed |
|
|
help_get_queries --> _get_queries (convention for private methods in Python) |
|
|
Actually, seems like it would make more sense to remove the queries argument and instead of inside: queries = HiveQuery.objects.using('query') |
|
|
if filter['text']: |
|
|
We should avoid any DB specific properties (the whole idea of the ORM is to be compatible with DBs other ... |
|
|
Could you add a test for the pagination? def test_search_pagination(self): with patch('jobbrowser.apis.hive_query_api.HiveQueryClient.get_queries') as get_queries: ... get_queries.return_value = [{.....}, {.....}] |
|
|
--> test_search_pagination |
|
|
Move to TestHiveClient()? Testing the API means whe nwe call the API, e.g. self.client.post("/jobbrowser/api/jobs/queries-hive", content_type='applecation/json', data=filters) |
|
|
nit: could move it up above line 204 and then below we can reuse query1.query_id = 'query_id |
|
-
Nice one!
Minor comments below and add minimal unit tests?
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api.py (Diff revision 2) help_get_queries --> _get_queries
(convention for private methods in Python)
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api.py (Diff revision 2) Actually,
seems like it would make more sense to remove the
queries
argument and instead of inside:queries = HiveQuery.objects.using('query')
-
-
-
desktop/core/src/desktop/settings.py (Diff revision 2) We should avoid any DB specific properties (the whole idea of the ORM is to be compatible with DBs other that psql). Regular prefix search is fine for v1 (and free text search require usability/perf tests/multi db testing... which is pretty much a lot more work to guarantee a stable experience)
Description: |
|
---|
Description: |
|
---|
-
Nice!
Nit, bit more explicit: HUE-9492 [query browser] Text search, date filtering and pagination
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api_tests.py (Diff revision 3) Could you add a test for the pagination? def test_search_pagination(self): with patch('jobbrowser.apis.hive_query_api.HiveQueryClient.get_queries') as get_queries: ... get_queries.return_value = [{.....}, {.....}]
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+129 -21) |
-
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api_tests.py (Diff revision 4) --> test_search_pagination
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api_tests.py (Diff revision 4) Move to TestHiveClient()?
Testing the API means whe nwe call the API, e.g. self.client.post("/jobbrowser/api/jobs/queries-hive", content_type='applecation/json', data=filters)
-
apps/jobbrowser/src/jobbrowser/apis/hive_query_api_tests.py (Diff revision 4) nit: could move it up above line 204
and then below we can reusequery1.query_id = 'query_id