-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this 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?
c610f07
to
03535f1
Compare
Nice catch! I have made frontend render of that. |
There was a problem hiding this 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
setSuggestions( | ||
json.map((suggestion: null | number | boolean | string) => ({ | ||
value: suggestion, | ||
label: optionLabel(suggestion), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
there's a related PR, I will work together with @stephenLYZ to improve this. |
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:
we need to add to escape mechanism somewhere in separated PR.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
Before
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
NULL
andempty string
#18147