iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(explore): #10098 boolean filter not working (#14567) 

* Restrict operators when column is boolean

* refactor 'isOperatorRelevant' a little bit

* Include 'BOOLEAN' to handle presto

* Update tests

* number column should show bool operators

* fix test - some dbs translate true/false to 1/0

* Fix tests and add linting

* When column type is boolean, show bool operators

* Address PR comments - simplify conditions

* Fix a linting error

* Addressing PR comment - remove unused variables
This commit is contained in:
Ajay M 2021-05-18 17:39:19 -04:00 committed by GitHub
parent 84e8dc71f6
commit 90378ed94e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 131 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js
View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { CUSTOM_OPERATORS } from 'src/explore/constants';
import { CUSTOM_OPERATORS, OPERATORS } from 'src/explore/constants';
import { getSimpleSQLExpression } from 'src/explore/exploreUtils';
export const EXPRESSION_TYPES = {
@ -42,6 +42,8 @@ const OPERATORS_TO_SQL = {
REGEX: 'REGEX',
'IS NOT NULL': 'IS NOT NULL',
'IS NULL': 'IS NULL',
'IS TRUE': 'IS TRUE',
'IS FALSE': 'IS FALSE',
'LATEST PARTITION': ({ datasource }) =>
`= '{{ presto.latest_partition('${datasource.schema}.${datasource.datasource_name}') }}'`,
};
@ -116,7 +118,14 @@ export default class AdhocFilter {
isValid() {
if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
if (this.operator === 'IS NOT NULL' || this.operator === 'IS NULL') {
if (
[
OPERATORS['IS TRUE'],
OPERATORS['IS FALSE'],
OPERATORS['IS NULL'],
OPERATORS['IS NOT NULL'],
].indexOf(this.operator) >= 0
) {
return !!(this.operator && this.subject);
}

78
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.jsx
View File

@ -196,4 +196,82 @@ describe('AdhocFilterEditPopoverSimpleTabContent', () => {
}),
);
});
it('will display boolean operators only when column type is boolean', () => {
const { wrapper } = setup({
datasource: {
type: 'table',
datasource_name: 'table1',
schema: 'schema',
columns: [{ column_name: 'value', type: 'BOOL' }],
},
adhocFilter: simpleAdhocFilter,
});
const booleanOnlyOperators = [
'IS TRUE',
'IS FALSE',
'IS NULL',
'IS NOT NULL',
];
booleanOnlyOperators.forEach(operator => {
expect(wrapper.instance().isOperatorRelevant(operator, 'value')).toBe(
true,
);
});
});
it('will display boolean operators when column type is number', () => {
const { wrapper } = setup({
datasource: {
type: 'table',
datasource_name: 'table1',
schema: 'schema',
columns: [{ column_name: 'value', type: 'INT' }],
},
adhocFilter: simpleAdhocFilter,
});
const booleanOnlyOperators = ['IS TRUE', 'IS FALSE'];
booleanOnlyOperators.forEach(operator => {
expect(wrapper.instance().isOperatorRelevant(operator, 'value')).toBe(
true,
);
});
});
it('will not display boolean operators when column type is string', () => {
const { wrapper } = setup({
datasource: {
type: 'table',
datasource_name: 'table1',
schema: 'schema',
columns: [{ column_name: 'value', type: 'STRING' }],
},
adhocFilter: simpleAdhocFilter,
});
const booleanOnlyOperators = ['IS TRUE', 'IS FALSE'];
booleanOnlyOperators.forEach(operator => {
expect(wrapper.instance().isOperatorRelevant(operator, 'value')).toBe(
false,
);
});
});
it('will display boolean operators when column is an expression', () => {
const { wrapper } = setup({
datasource: {
type: 'table',
datasource_name: 'table1',
schema: 'schema',
columns: [
{
column_name: 'value',
expression: 'case when value is 0 then "NO"',
},
],
},
adhocFilter: simpleAdhocFilter,
});
const booleanOnlyOperators = ['IS TRUE', 'IS FALSE'];
booleanOnlyOperators.forEach(operator => {
expect(wrapper.instance().isOperatorRelevant(operator, 'value')).toBe(
true,
);
});
});
});

23
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx
View File

@ -235,11 +235,30 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
}
isOperatorRelevant(operator, subject) {
const column = this.props.datasource.columns?.find(
col => col.column_name === subject,
);
const isColumnBoolean =
!!column && (column.type === 'BOOL' || column.type === 'BOOLEAN');
const isColumnNumber = !!column && column.type === 'INT';
const isColumnFunction = !!column && !!column.expression;
if (operator && CUSTOM_OPERATORS.has(operator)) {
const { partitionColumn } = this.props;
return partitionColumn && subject && subject === partitionColumn;
}
if (
operator === OPERATORS['IS TRUE'] ||
operator === OPERATORS['IS FALSE']
) {
return isColumnBoolean || isColumnNumber || isColumnFunction;
}
if (isColumnBoolean) {
return (
operator === OPERATORS['IS NULL'] ||
operator === OPERATORS['IS NOT NULL']
);
}
return !(
(this.props.datasource.type === 'druid' &&
TABLE_ONLY_OPERATORS.indexOf(operator) >= 0) ||
@ -318,7 +337,7 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
OPERATORS_OPTIONS.filter(op => this.isOperatorRelevant(op, subject))
.length,
),
// like AGGREGTES_OPTIONS, operator options are string
// like AGGREGATES_OPTIONS, operator options are string
value: operator,
onChange: this.onOperatorChange,
filterOption: (input, option) =>

5
superset-frontend/src/explore/constants.ts
View File

@ -42,7 +42,10 @@ export const OPERATORS = {
'IS NOT NULL': 'IS NOT NULL',
'IS NULL': 'IS NULL',
'LATEST PARTITION': 'LATEST PARTITION',
'IS TRUE': 'IS TRUE',
'IS FALSE': 'IS FALSE',
};
export const OPERATORS_OPTIONS = Object.values(OPERATORS);
export const TABLE_ONLY_OPERATORS = [OPERATORS.LIKE];
@ -65,6 +68,8 @@ export const DISABLE_INPUT_OPERATORS = [
OPERATORS['IS NOT NULL'],
OPERATORS['IS NULL'],
OPERATORS['LATEST PARTITION'],
OPERATORS['IS TRUE'],
OPERATORS['IS FALSE'],
];
export const sqlaAutoGeneratedMetricNameRegex = /^(sum|min|max|avg|count|count_distinct)__.*$/i;

4
superset/connectors/sqla/models.py
View File

@ -1205,6 +1205,10 @@ class SqlaTable( # pylint: disable=too-many-public-methods,too-many-instance-at
where_clause_and.append(col_obj.get_sqla_col().is_(None))
elif op == utils.FilterOperator.IS_NOT_NULL.value:
where_clause_and.append(col_obj.get_sqla_col().isnot(None))
elif op == utils.FilterOperator.IS_TRUE.value:
where_clause_and.append(col_obj.get_sqla_col().is_(True))
elif op == utils.FilterOperator.IS_FALSE.value:
where_clause_and.append(col_obj.get_sqla_col().is_(False))
else:
if eq is None:
raise QueryObjectValidationError(

2
superset/utils/core.py
View File

@ -211,6 +211,8 @@ class FilterOperator(str, Enum):
IN = "IN" # pylint: disable=invalid-name
NOT_IN = "NOT IN"
REGEX = "REGEX"
IS_TRUE = "IS TRUE"
IS_FALSE = "IS FALSE"
class PostProcessingBoxplotWhiskerType(str, Enum):

12
tests/sqla_models_tests.py
View File

@ -169,11 +169,14 @@ class TestDatabaseModel(SupersetTestCase):
class FilterTestCase(NamedTuple):
operator: str
value: Union[float, int, List[Any], str]
expected: str
expected: Union[str, List[str]]
filters: Tuple[FilterTestCase, ...] = (
FilterTestCase(FilterOperator.IS_NULL, "", "IS NULL"),
FilterTestCase(FilterOperator.IS_NOT_NULL, "", "IS NOT NULL"),
# Some db backends translate true/false to 1/0
FilterTestCase(FilterOperator.IS_TRUE, "", ["IS 1", "IS true"]),
FilterTestCase(FilterOperator.IS_FALSE, "", ["IS 0", "IS false"]),
FilterTestCase(FilterOperator.GREATER_THAN, 0, "> 0"),
FilterTestCase(FilterOperator.GREATER_THAN_OR_EQUALS, 0, ">= 0"),
FilterTestCase(FilterOperator.LESS_THAN, 0, "< 0"),
@ -199,7 +202,12 @@ class TestDatabaseModel(SupersetTestCase):
}
sqla_query = table.get_sqla_query(**query_obj)
sql = table.database.compile_sqla_query(sqla_query.sqla_query)
self.assertIn(filter_.expected, sql)
if isinstance(filter_.expected, list):
self.assertTrue(
any([candidate in sql for candidate in filter_.expected])
)
else:
self.assertIn(filter_.expected, sql)
def test_incorrect_jinja_syntax_raises_correct_exception(self):
query_obj = {