Skip to content
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

fix: boolean type into SQL 'in' operator #16107

Merged
merged 5 commits into from
Aug 10, 2021

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Aug 6, 2021

SUMMARY

closes: #16081
Currently, there is an issue with filters that handle boolean values. If the database engine does not provide boolean value implicit conversion, will cause the query to fail.
Imagine SQL:

SELECT case
           when gender = 'boy' then True
           else False
       end AS boolean_gender,
       count(num) AS "COUNT(num)"
FROM birth_names
WHERE case
          when gender = 'boy' then True
          else False
      end IN ('false')
GROUP BY case
             when gender = 'boy' then True
             else False
         end
ORDER BY "COUNT(num)" DESC
LIMIT 10000;

the derived column(Calculated column in Superset) type is Boolean, and the operand is String('true')

case
    when gender = 'boy' then True
    else False
end IN ('false')

This PR introduced a conversion when the column type is boolean and operand type also is boolean.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

boolean.value.into.in.operator.mov

TESTING INSTRUCTIONS

  1. select birth_names dataset
  2. create a new Calculated Columns.
    name: boolean_gender
    SQL expression : case when gender = 'boy' then True else False end
    Data Type: Boolean
  3. create a treemap v2. select boolean_gender into Group by control; select sum(num) into Metric; check emit dashboard cross filters; saved to new dashboard test
  4. create a table. select boolean_gender into Group by control; select sum(num) into Metric; check emit dashboard cross filters; saved to dashboard test
  5. goto dashboard test
  6. select true or false in treemap v2, then the table can be filtered.
  7. double-check view query in table viz, where clause like below:
WHERE case
          when gender = 'boy' then True
          else False
      end IN (false)

ADDITIONAL INFORMATION

  • Has associated issue: [cross-filtering] treemap not emitting boolean column correctly #16081
  • 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

@@ -112,7 +112,7 @@ const ColumnButtonWrapper = styled.div`
const checkboxGenerator = (d, onChange) => (
<CheckboxControl value={d} onChange={onChange} />
);
const DATA_TYPES = ['STRING', 'NUMERIC', 'DATETIME'];
const DATA_TYPES = ['STRING', 'NUMERIC', 'DATETIME', 'BOOLEAN'];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bycatch: added boolean type in the UI

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #16107 (de3adaa) into master (490890d) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16107      +/-   ##
==========================================
- Coverage   76.88%   76.62%   -0.27%     
==========================================
  Files         995      995              
  Lines       52864    52892      +28     
  Branches     6712     6721       +9     
==========================================
- Hits        40644    40528     -116     
- Misses      11994    12139     +145     
+ Partials      226      225       -1     
Flag Coverage Δ
hive ?
javascript 71.21% <100.00%> (-0.09%) ⬇️
mysql 81.60% <100.00%> (+0.04%) ⬆️
postgres 81.62% <100.00%> (+<0.01%) ⬆️
presto ?
python 81.71% <100.00%> (-0.43%) ⬇️
sqlite 81.22% <100.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...erset-frontend/src/datasource/DatasourceEditor.jsx 74.25% <100.00%> (ø)
superset/connectors/base/models.py 88.14% <100.00%> (+0.07%) ⬆️
superset/utils/core.py 88.18% <100.00%> (-0.04%) ⬇️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
superset/db_engine_specs/hive.py 69.80% <0.00%> (-16.87%) ⬇️
...ontrols/DndColumnSelectControl/DndColumnSelect.tsx 46.42% <0.00%> (-14.69%) ⬇️
...ols/DndColumnSelectControl/utils/optionSelector.ts 34.61% <0.00%> (-11.54%) ⬇️
superset/db_engine_specs/presto.py 83.47% <0.00%> (-6.49%) ⬇️
...onalFormattingControl/FormattingPopoverContent.tsx 29.26% <0.00%> (-4.07%) ⬇️
...src/dashboard/components/PropertiesModal/index.jsx 83.09% <0.00%> (-2.52%) ⬇️
... and 17 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 490890d...de3adaa. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2021

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one non-blocking recommendation

Comment on lines +439 to +440
>>> cast_to_boolean('False')
False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would also add this as it seems to be missing

>>> cast_to_boolean('True')
True

@zhaoyongjie zhaoyongjie merged commit bb1d8fe into apache:master Aug 10, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rosemarie-chiu
Copy link
Contributor

🏷 2021.31

stevenuray pushed a commit to preset-io/superset that referenced this pull request Aug 11, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line

(cherry picked from commit bb1d8fe)
@villebro villebro added the v1.3 label Aug 12, 2021
villebro pushed a commit that referenced this pull request Aug 16, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line

(cherry picked from commit bb1d8fe)
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line

(cherry picked from commit bb1d8fe)
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line

(cherry picked from commit bb1d8fe)
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* fix: boolean type into SQL 'in' operator

* fix ut

* fix ut again

* update url

* remove blank line

(cherry picked from commit dde34b0)
@mistercrunch mistercrunch added 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2021.31 size/M v1.3 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cross-filtering] treemap not emitting boolean column correctly
4 participants