-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[hotfix] Fix filter for sqlalchemy and druid #2293
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,9 @@ import Select from 'react-select'; | |
import { Button, Row, Col } from 'react-bootstrap'; | ||
import SelectControl from './SelectControl'; | ||
|
||
const arrayFilterOps = ['in', 'not in']; | ||
const strFilterOps = ['==', '!=', '>', '<', '>=', '<=', 'regex']; | ||
|
||
const propTypes = { | ||
choices: PropTypes.array, | ||
changeFilter: PropTypes.func, | ||
|
@@ -47,6 +50,21 @@ export default class Filter extends React.Component { | |
}); | ||
} | ||
} | ||
switchFilterValue(prevFilter, nextOp) { | ||
const prevOp = prevFilter.op; | ||
let newVal = null; | ||
if (arrayFilterOps.indexOf(prevOp) !== -1 | ||
&& strFilterOps.indexOf(nextOp) !== -1) { | ||
// switch from array to string | ||
newVal = this.props.filter.val.length > 0 ? this.props.filter.val[0] : ''; | ||
} | ||
if (strFilterOps.indexOf(prevOp) !== -1 | ||
&& arrayFilterOps.indexOf(nextOp) !== -1) { | ||
// switch from string to array | ||
newVal = this.props.filter.val === '' ? [] : [this.props.filter.val]; | ||
} | ||
return newVal; | ||
} | ||
changeFilter(control, event) { | ||
let value = event; | ||
if (event && event.target) { | ||
|
@@ -55,7 +73,16 @@ export default class Filter extends React.Component { | |
if (event && event.value) { | ||
value = event.value; | ||
} | ||
this.props.changeFilter(control, value); | ||
if (control === 'op') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const newVal = this.switchFilterValue(this.props.filter, value); | ||
if (newVal) { | ||
this.props.changeFilter(['op', 'val'], [value, newVal]); | ||
} else { | ||
this.props.changeFilter(control, value); | ||
} | ||
} else { | ||
this.props.changeFilter(control, value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
} | ||
if (control === 'col' && value !== null && this.props.datasource.filter_select) { | ||
this.fetchFilterValues(value); | ||
} | ||
|
@@ -70,13 +97,13 @@ export default class Filter extends React.Component { | |
this.fetchFilterValues(filter.col); | ||
} | ||
} | ||
if (this.props.having) { | ||
// druid having filter | ||
if (strFilterOps.indexOf(filter.op) !== -1) { | ||
// druid having filter or regex/==/!= filters | ||
return ( | ||
<input | ||
type="text" | ||
onChange={this.changeFilter.bind(this, 'val')} | ||
value={filter.value} | ||
value={filter.val} | ||
className="form-control input-sm" | ||
placeholder="Filter value" | ||
/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1403,7 +1403,8 @@ def visit_column(element, compiler, **kw): | |
col_obj = cols.get(col) | ||
if col_obj and op in ('in', 'not in'): | ||
values = [types.strip("'").strip('"') for types in eq] | ||
values = [utils.js_string_to_num(s) for s in values] | ||
if col_obj.is_num: | ||
values = [utils.js_string_to_num(s) for s in values] | ||
cond = col_obj.sqla_col.in_(values) | ||
if op == 'not in': | ||
cond = ~cond | ||
|
@@ -2567,8 +2568,7 @@ def increment_timestamp(ts): | |
query=query_str, | ||
duration=datetime.now() - qry_start_dttm) | ||
|
||
@staticmethod | ||
def get_filters(raw_filters): | ||
def get_filters(self, raw_filters): | ||
filters = None | ||
for flt in raw_filters: | ||
if not all(f in flt for f in ['col', 'op', 'val']): | ||
|
@@ -2577,21 +2577,24 @@ def get_filters(raw_filters): | |
op = flt['op'] | ||
eq = flt['val'] | ||
cond = None | ||
if op in ('in', 'not in'): | ||
eq = [types.replace("'", '').strip() for types in eq] | ||
if col in self.num_cols: | ||
if op in ('in', 'not in'): | ||
eq = [utils.js_string_to_num(v) for v in eq] | ||
else: | ||
eq = utils.js_string_to_num(eq) | ||
if op == '==': | ||
cond = Dimension(col) == eq | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't need to provide numbers here if comparing to a numerical column? |
||
elif op == '!=': | ||
cond = ~(Dimension(col) == eq) | ||
elif op in ('in', 'not in'): | ||
fields = [] | ||
# Distinguish quoted values with regular value types | ||
values = [types.replace("'", '') for types in eq] | ||
values = [utils.js_string_to_num(s) for s in values] | ||
if len(values) > 1: | ||
for s in values: | ||
s = s.strip() | ||
if len(eq) > 1: | ||
for s in eq: | ||
fields.append(Dimension(col) == s) | ||
cond = Filter(type="or", fields=fields) | ||
elif len(values) == 1: | ||
elif len(eq) == 1: | ||
cond = Dimension(col) == eq[0] | ||
if op == 'not in': | ||
cond = ~cond | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the same code is repeated twice here (55:57 and 58:60)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first event type is for input event, the second is for SelectField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. overall it would be more clear to have two functions, one for
changeOp
and one forchangeValue