-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
feat: support None operand in EQUAL operator #21713
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21713 +/- ##
==========================================
- Coverage 66.84% 66.82% -0.03%
==========================================
Files 1799 1799
Lines 68881 68881
Branches 7319 7319
==========================================
- Hits 46044 46028 -16
- Misses 20950 20966 +16
Partials 1887 1887
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
result_object = table.query( | ||
@only_postgresql | ||
def test_should_generate_closed_and_open_time_filter_range(login_as_admin): |
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.
bycatch: use only_postgresql
decorator to narrow the test case, and use login_as_admin
to define the Flask app context, there's no logical change.
def test_none_operand_in_filter(login_as_admin, physical_dataset): | ||
expected_results = [ |
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 test case is the new one.
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.
I hadn't noticed it actually allowed selecting the empty value despite the API not allowing it 😵💫 Works great! However, one bug that I noticed when testing + a few stylistic nits:
-
When creating a new filter, then selecting "IS NOT NULL" operator and then switching back to "equals" or "not equals", the save button becomes disabled:
-
We should also update the pill labels to reflect that they are checking for NULL. For the "equals" case I'd do something like "col = NULL" and "col <> NULL":
-
Also a bycatch, but I'd actually prefer for the NOT EQUALS operator to use the same ≠ character in the pill label that we're using in the dropdown:
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 - leaving my comments on the table for a potential follow-up PR as they're mostly unrelated
@villebro Thanks a lot, I will fix the frontend problem in the separated PR. |
@zhaoyongjie did you address the frontend issues that @villebro mentioned in a separate PR? If so could you reference this PR in said PR so there's a edge connecting the two PRs? |
@john-bodley I will do these cases. thanks for the mention. |
This reverts commit 05648eb.
This reverts commit 05648eb.
SUMMARY
This PR intends to support the
None
operand withEqual
andNot-Equal
operators in Filters in QueryObject. Currently, theIn
operator has supportedNone
operand calculation, in order to align that theEqual
andNot-Equal
should support as well.After the PR, there are 4 approaches to generate
is null
oris not null
filter in SQL.EQUAL
/NOT EQUAL
operatorIS NULL
/IS NOT NULL
operatorIN
operator<NULL>
as filter operand literal constant in the EQ or IN operatorsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION