Add parenthesis for custom where and having #1337
Conversation
| @@ -486,3 +486,15 @@ def __exit__(self, type, value, traceback): | |||
| except ValueError as e: | |||
| logging.warning("timeout can't be used in the current context") | |||
| logging.exception(e) | |||
|
|
|||
There was a problem hiding this comment.
PEP8 says 2 blank lines before a module scoped function
| @@ -55,7 +55,7 @@ | |||
| from caravel import app, db, db_engine_specs, get_session, utils, sm | |||
| from caravel.source_registry import SourceRegistry | |||
| from caravel.viz import viz_types | |||
| from caravel.utils import flasher, MetricPermException, DimSelector | |||
| from caravel.utils import flasher, MetricPermException, DimSelector, wrap_clause | |||
There was a problem hiding this comment.
line is 1 char too long according to code climate...
| @@ -486,3 +486,15 @@ def __exit__(self, type, value, traceback): | |||
| except ValueError as e: | |||
| logging.warning("timeout can't be used in the current context") | |||
| logging.exception(e) | |||
|
|
|||
| def wrap_clause(sql): | |||
There was a problem hiding this comment.
the doctest fails because you return a sqlalchemy.text object.
I'd make this function more generic and call it wrap_in_parentheses(s) and wrap clauses systematically (regardless of the OR in there). Note that the 'OR' in currently would trigger on something like 'my_field_has_the_letter_or_in_it > 0', but that's fine, we should just always put parentheses around the user clause when provided
|
@mistercrunch I've updated the code according to your comments. Please review again. travis-ci timed out when building javascript (that should have nothing to do with this pull request) |
|
Just rebased from master. And this travis-ci doesn't timeout either. @mistercrunch could you review this and merge? Thanks. |
|
Thanks for the PR! |
mistercrunch
added
🏷️ bot
Currently, when there's a 'OR' in custom where, it may result an unexpected query.
For example, if a user explore a table having a main dttm field, and he sets custom where as
cond1="xxx" or cond2="yyy". The query generated would beThat means any records that met
cond="yyy"would be counted, even those records are out of user-specified date range. That result might not be what user expected. It would be much more reasonable if the generated query is