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

m-ajay · 2021-05-11T15:12:22Z

SUMMARY

Fixes #10098.
When the column type is boolean/number type, show IS TRUE and IS FALSE options.

TEST PLAN

CI and manual test

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

junlincc · 2021-05-11T15:39:53Z

hi ajay, thanks for contributing to the quality bash! please make sure to include screenshot and description in your pr and change the title. e.g. fix(explore): filter on boolean column ...

graceguo-supercat · 2021-05-11T22:21:30Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx

+        0
+      );
+    }
+    if (dataSourceType === 'table' && !isColumnNumber) {


For calculated column, which could be number, string, date, or unset type. If it is unset, we probably show IS_TRUE, IS_FALSE as option?

Here is an example of setting for calculated column:

ktmud

I don't think we need to add additional comparators. Superset should be smart enough to know a column is boolean or not.

The col_spec accessed earlier already has an indication whether a column is Boolean. You can just handle the filter value differently based on this information.

ktmud · 2021-05-11T22:38:05Z

The easier fix is probably convert string value "true"/"false" to booleans here.

An optional work is to update the frontend to change the input from text input to select.

graceguo-supercat · 2021-05-11T22:59:28Z

I don't think we need to additional comparators. Superset should be smart enough to know a column is boolean or not.

An optional work is to update the frontend to change the input from text input to select.

If not the additional operator, IS_TURE, IS_FALSE, which are 2 new options for the select, what is suggested way for the frontend to change the input from text input to select?

ktmud · 2021-05-12T17:00:00Z

IIRC, currently the value input would query the backend for available options, we can probably just give it a fixed options if we detect the column type is boolean.

codecov · 2021-05-12T20:40:18Z

Codecov Report

Merging #14567 (32ebf1c) into master (adbe56c) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #14567      +/-   ##
==========================================
- Coverage   77.40%   77.32%   -0.08%     
==========================================
  Files         958      958              
  Lines       48326    48579     +253     
  Branches     5677     5713      +36     
==========================================
+ Hits        37407    37565     +158     
- Misses      10719    10814      +95     
  Partials      200      200

Flag	Coverage Δ
hive	`?`
javascript	`72.54% <100.00%> (+0.07%)`	⬆️
mysql	`81.39% <100.00%> (+0.23%)`	⬆️
postgres	`81.41% <100.00%> (+0.22%)`	⬆️
presto	`?`
python	`81.49% <100.00%> (-0.23%)`	⬇️
sqlite	`81.03% <100.00%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/explore/constants.ts	`100.00% <ø> (ø)`
...onents/controls/FilterControl/AdhocFilter/index.js	`97.01% <100.00%> (+0.09%)`	⬆️
...l/AdhocFilterEditPopoverSimpleTabContent/index.jsx	`79.39% <100.00%> (+1.91%)`	⬆️
superset/connectors/sqla/models.py	`88.46% <100.00%> (-1.62%)`	⬇️
superset/utils/core.py	`88.90% <100.00%> (-0.10%)`	⬇️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
...uperset-frontend/src/components/Form/FormLabel.tsx	`68.75% <0.00%> (-31.25%)`	⬇️
superset/db_engine_specs/hive.py	`70.32% <0.00%> (-17.08%)`	⬇️
...ontend/src/common/hooks/apiResources/dashboards.ts	`40.00% <0.00%> (-10.00%)`	⬇️
...ponents/controls/TimeSeriesColumnControl/index.jsx	`91.78% <0.00%> (-8.22%)`	⬇️
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adbe56c...32ebf1c. Read the comment docs.

junlincc · 2021-05-13T01:08:34Z

/testenv up

github-actions · 2021-05-13T01:10:31Z

@junlincc Ephemeral environment spinning up at http://52.32.196.39:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

graceguo-supercat

LGTM

ktmud · 2021-05-13T19:42:50Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx

+      if (dataSourceType === 'table') {
+        return !(DRUID_ONLY_OPERATORS.indexOf(operator) >= 0);
+      }
+    }


I think this whole section can be simplified as

if (operator === OPERATORS['IS TRUE'] || operator === OPERATORS['IS FALSE'])) { return isColumnBoolean || isColumnBoolean; } if (isColumnBoolean) { return operator === OPERATORS['IS NULL'] || operator === OPERATORS['IS NOT NULL']; }

geido

LGTM!

ktmud

LGTM with just a couple of notes on cleanups.

ktmud · 2021-05-18T14:48:35Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.jsx

-        DRUID_ONLY_OPERATORS.indexOf(operator) >= 0) ||
+        [...BOOLEAN_ONLY_OPERATORS, ...DRUID_ONLY_OPERATORS].indexOf(
+          operator,
+        ) >= 0) ||


You can revert changes in these two lines, too, since you've checked boolean operators above.

ktmud · 2021-05-18T14:49:14Z

superset-frontend/src/explore/constants.ts

+export const BOOLEAN_ONLY_OPERATORS = [
+  OPERATORS['IS TRUE'],
+  OPERATORS['IS FALSE'],
+];


This variable could also be deleted.

github-actions · 2021-05-19T01:20:28Z

Ephemeral environment shutdown and build artifacts deleted.

junlincc · 2021-05-20T05:25:42Z

@m-ajay thanks again for the fix! i believe boolean filter is broken in filter box as well, does this change cover that?

* 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

m-ajay added 2 commits May 10, 2021 18:23

Restrict operators when column is boolean

b74e312

refactor 'isOperatorRelevant' a little bit

c45ec02

pull-request-size bot added the size/M label May 11, 2021

junlincc added need:more-info Requires more information from author need: change-title labels May 11, 2021

Include 'BOOLEAN' to handle presto

f25d0fb

m-ajay changed the title ~~[WIP] Fix/issue 10098 boolean filter not working~~ fix: [WIP] #10098 boolean filter not working May 11, 2021

Update tests

682f291

graceguo-supercat changed the title ~~fix: [WIP] #10098 boolean filter not working~~ fix: #10098 boolean filter not working May 11, 2021

graceguo-supercat marked this pull request as ready for review May 11, 2021 22:19

graceguo-supercat reviewed May 11, 2021

View reviewed changes

ktmud reviewed May 11, 2021

View reviewed changes

number column should show bool operators

9024352

m-ajay changed the title ~~fix: #10098 boolean filter not working~~ fix(explore): #10098 boolean filter not working May 12, 2021

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

541b3b8

Fix tests and add linting

24c0161

pull-request-size bot added size/L and removed size/M labels May 12, 2021

When column type is boolean, show bool operators

cef4926

junlincc removed need: change-title need:more-info Requires more information from author labels May 13, 2021

m-ajay requested a review from ktmud May 13, 2021 13:45

m-ajay requested a review from graceguo-supercat May 13, 2021 13:45

graceguo-supercat approved these changes May 13, 2021

View reviewed changes

ktmud reviewed May 13, 2021

View reviewed changes

Address PR comments - simplify conditions

a3f6b76

m-ajay requested a review from ktmud May 17, 2021 14:23

geido approved these changes May 17, 2021

View reviewed changes

Fix a linting error

f5373ff

ktmud approved these changes May 18, 2021

View reviewed changes

Addressing PR comment - remove unused variables

32ebf1c

graceguo-supercat merged commit 90378ed into apache:master May 18, 2021

m-ajay deleted the fix/issue-10098-boolean-filter-not-working branch May 19, 2021 19:54

m-ajay mentioned this pull request Jun 2, 2021

fix(explore): Explore page boolean filter is broken for Presto DB #14952

Merged

8 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

m-ajay commented May 11, 2021 •

edited by graceguo-supercat

junlincc commented May 11, 2021

graceguo-supercat May 11, 2021 •

edited

ktmud left a comment •

edited

ktmud commented May 11, 2021

graceguo-supercat commented May 11, 2021

ktmud commented May 12, 2021

codecov bot commented May 12, 2021 •

edited

junlincc commented May 13, 2021

github-actions bot commented May 13, 2021

graceguo-supercat left a comment

ktmud May 13, 2021

geido left a comment

ktmud left a comment

ktmud May 18, 2021

ktmud May 18, 2021

github-actions bot commented May 19, 2021

junlincc commented May 20, 2021 •

edited

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

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

Conversation

m-ajay commented May 11, 2021 • edited by graceguo-supercat

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

junlincc commented May 11, 2021

graceguo-supercat May 11, 2021 • edited

Choose a reason for hiding this comment

ktmud left a comment • edited

Choose a reason for hiding this comment

ktmud commented May 11, 2021

graceguo-supercat commented May 11, 2021

ktmud commented May 12, 2021

codecov bot commented May 12, 2021 • edited

Codecov Report

junlincc commented May 13, 2021

github-actions bot commented May 13, 2021

graceguo-supercat left a comment

Choose a reason for hiding this comment

ktmud May 13, 2021

Choose a reason for hiding this comment

geido left a comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud May 18, 2021

Choose a reason for hiding this comment

ktmud May 18, 2021

Choose a reason for hiding this comment

github-actions bot commented May 19, 2021

junlincc commented May 20, 2021 • edited

m-ajay commented May 11, 2021 •

edited by graceguo-supercat

graceguo-supercat May 11, 2021 •

edited

ktmud left a comment •

edited

codecov bot commented May 12, 2021 •

edited

junlincc commented May 20, 2021 •

edited