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: null value and empty string in filter #18171

Merged
merged 5 commits into from
Jan 27, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jan 26, 2022

SUMMARY

closes: #18147
Currently, Superset Filter can't show NULL value and empty string. This PR replaced NULL with <NULL> and replaced "" with <empty string>

Notice that: A known limitation for the current design, users can't filter below text values:

- "<empty string>"
- "<NULL>"

we need to add to escape mechanism somewhere in separated PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

image

Before

image

TESTING INSTRUCTIONS

  1. Create a virtual dataset
SELECT '' as name
UNION ALL
SELECT null as name
union all
SELECT 'foo' as name
  1. Open the virtual dataset in Explore Page
  2. Drag name column in Filter Control
  3. You can get 3 options in the filter selection
  4. Add all options and open query inspector. The query is:
SELECT count(*) AS count
FROM
  (SELECT '' as name
   UNION ALL SELECT null as name
   union all SELECT 'foo' as name) AS virtual_table
WHERE name IS NULL
  OR name IN ('',
              'foo',)
ORDER BY count DESC
LIMIT 1000;

ADDITIONAL INFORMATION

  • Has associated issue: [explore] Filter unable to select NULL and empty string #18147
  • Required feature flags:
  • 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

@zhaoyongjie zhaoyongjie requested review from villebro and betodealmeida and removed request for villebro January 26, 2022 08:24
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #18171 (c52b6e3) into master (4b89ac7) will decrease coverage by 0.07%.
The diff coverage is 84.61%.

❗ Current head c52b6e3 differs from pull request most recent head 024d529. Consider uploading reports for the commit 024d529 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18171      +/-   ##
==========================================
- Coverage   65.95%   65.87%   -0.08%     
==========================================
  Files        1584     1591       +7     
  Lines       62046    62416     +370     
  Branches     6273     6286      +13     
==========================================
+ Hits        40920    41119     +199     
- Misses      19505    19675     +170     
- Partials     1621     1622       +1     
Flag Coverage Δ
hive ?
mysql 81.28% <92.56%> (+0.10%) ⬆️
postgres 81.33% <92.56%> (+0.10%) ⬆️
presto ?
python 81.42% <92.56%> (-0.25%) ⬇️
sqlite 81.03% <92.56%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
.../superset-ui-core/src/connection/SupersetClient.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...chart-echarts/src/MixedTimeseries/controlPanel.tsx 80.00% <ø> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <0.00%> (ø)
...lugin-chart-echarts/src/Timeseries/transformers.ts 50.83% <ø> (ø)
superset-frontend/src/embedded/index.tsx 0.00% <0.00%> (ø)
...nd/src/explore/components/ExploreActionButtons.tsx 59.45% <0.00%> (-3.40%) ⬇️
...rc/explore/components/controls/TextAreaControl.jsx 80.00% <0.00%> (-4.22%) ⬇️
superset-frontend/src/preamble.ts 0.00% <ø> (ø)
superset-frontend/src/setup/setupClient.ts 0.00% <0.00%> (ø)
... and 66 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 4b89ac7...024d529. Read the comment docs.

@zhaoyongjie zhaoyongjie changed the title fix: null and empty string in filter fix: null value and empty string in filter Jan 26, 2022
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.

As a bandaid this should fix the current issue. However, I can't help but feel slightly uncomfortable by these types of workarounds. I think the optimal architectural design would be such, that <NULL> and <empty string> would be the labels in the select component in the frontend, but the value would still be retained in its original form. Did you consider this approach, and if so, was there some reason it doesn't work?

superset/constants.py Show resolved Hide resolved
@zhaoyongjie
Copy link
Member Author

As a bandaid this should fix the current issue. However, I can't help but feel slightly uncomfortable by these types of workarounds. I think the optimal architectural design would be such, that <NULL> and <empty string> would be the labels in the select component in the frontend, but the value would still be retained in its original form. Did you consider this approach, and if so, was there some reason it doesn't work?

Nice catch! I have made frontend render of that.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 27, 2022
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.

This is great! One last question regarding possible simplification of handle_single_value, other than that LGTM

superset/connectors/base/models.py Show resolved Hide resolved
superset/connectors/base/models.py Outdated Show resolved Hide resolved
superset/connectors/druid/models.py Outdated Show resolved Hide resolved
superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
tests/integration_tests/sqla_models_tests.py Show resolved Hide resolved
tests/integration_tests/sqla_models_tests.py Show resolved Hide resolved
setSuggestions(
json.map((suggestion: null | number | boolean | string) => ({
value: suggestion,
label: optionLabel(suggestion),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we translate the option labels? I notice that the optionLabel function is always returning the values in English.

Copy link
Member

Choose a reason for hiding this comment

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

..then the translation needs to be done where the constant is defined. And if we do that, then we should definitely remove support for using the strings "<NULL>" and "<empty string>" as proxies for None and "" respectively in the backend and assume they'll always just be labels, but never values.

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 - there's some additional ideas on how this could be further improved, but let's leave those to follow-up PRs to get this bug fixed first. Thanks @zhaoyongjie for all the iteration, much appreciated!

@zhaoyongjie
Copy link
Member Author

LGTM - there's some additional ideas on how this could be further improved, but let's leave those to follow-up PRs to get this bug fixed first. Thanks @zhaoyongjie for all the iteration, much appreciated!

there's a related PR, I will work together with @stephenLYZ to improve this.

@zhaoyongjie zhaoyongjie merged commit 20b4ae1 into apache:master Jan 27, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.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 size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[explore] Filter unable to select NULL and empty string
4 participants