From 90378ed94eedf2a100e565cf742bc76ff1597932 Mon Sep 17 00:00:00 2001 From: Ajay M Date: Tue, 18 May 2021 17:39:19 -0400 Subject: [PATCH] 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 --- .../FilterControl/AdhocFilter/index.js | 13 +++- ...FilterEditPopoverSimpleTabContent.test.jsx | 78 +++++++++++++++++++ .../index.jsx | 23 +++++- superset-frontend/src/explore/constants.ts | 5 ++ superset/connectors/sqla/models.py | 4 + superset/utils/core.py | 2 + tests/sqla_models_tests.py | 12 ++- 7 files changed, 131 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js index 6928f544c..a4f11da25 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js @@ -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); } diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.jsx index 7eb5036ea..51678db67 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.jsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.jsx @@ -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, + ); + }); + }); }); diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx index e1b4aad3b..836ba2390 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx @@ -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) => diff --git a/superset-frontend/src/explore/constants.ts b/superset-frontend/src/explore/constants.ts index 06fa62c31..d17eb0276 100644 --- a/superset-frontend/src/explore/constants.ts +++ b/superset-frontend/src/explore/constants.ts @@ -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; diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 0ad75c516..1b4343cd8 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -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( diff --git a/superset/utils/core.py b/superset/utils/core.py index 3e089580a..ea7604cd2 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -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): diff --git a/tests/sqla_models_tests.py b/tests/sqla_models_tests.py index 98e65f3bb..69dc4c737 100644 --- a/tests/sqla_models_tests.py +++ b/tests/sqla_models_tests.py @@ -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 = {