revert: feat: support None operand in EQUAL operator (#21713)"

[john-bodley]

SUMMARY

This PR reverts #21713 —via git revert. @zhaoyongjie and @villebro though I love the essence of this PR—which is somewhat akin to what Tableau does; making SQL more attainable especially as it relates to handling NULL—sadly I don't believe the change is ready for primetime. Specifically:

1. Aspects of feat: support None operand in EQUAL operator #21713 (review) haven't been addressed.
2. The None use case never materializes as far as I'm concerned (see attached screenshots) given that this is interpreted as the string 'None'.
3. The CUSTOM SQL tab in the ad-hoc filter modal doesn't reflect the SIMPLE tab logic and thus there's a disconnect between the SQL rendered in the modal and the executed SQL (see attached screenshots).
4. We already have the IS NULL and IS NOT NULL options which this functionality is replacing. Collectively as a community we need to decide whether the filters map one-to-one with SQL—which is currently how the ad-hoc filter model is defined given the symbiotic relationship between the SIMPLE and CUSTOM SQL tabs—or should depart from SQL and represent what is commonly perceived user intent, especially if users aren't well versed with the nuances when handling NULLs.

I think for consistency it first makes sense to revert said logic before the issues/inconsistencies have been addressed. Furthermore such a change could actually break existing charts—if the author was unaware of the nuances around NULL—and thus a line item in UPDATING.md is likely required so various installations can send out the appropriate PSA to their users.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Handling of None

SIMPLE

Executed SQL

SIMPLE vs CUSTOM SQL

SIMPLE

CUSTOM SQL

Executed SQL

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• 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

[villebro]

@john-bodley I believe the original reason for this PR was due to the fact that native filters or cross filters were misbehaving due to this inconsistency. Unless there are known regressions caused by this PR I'd prefer to see us incrementally improve the UX rather than moving back to the previous state where filtering with NULL values is broken again. @rusackas @michael-s-molina who might have additional context.

[codecov]

Codecov Report

Merging #22065 (9546178) into master (cd1b379) will decrease coverage by 11.34%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #22065       +/-   ##
===========================================
- Coverage   67.07%   55.72%   -11.35%     
===========================================
  Files        1815     1815               
  Lines       69575    69575               
  Branches     7486     7486               
===========================================
- Hits        46665    38771     -7894     
- Misses      20974    28868     +7894     
  Partials     1936     1936               

Flag	Coverage Δ
hive	52.82% <100.00%> (ø)
mysql	?
postgres	?
presto	52.73% <100.00%> (ø)
python	58.03% <100.00%> (-23.54%)	⬇️
sqlite	?
unit	51.20% <0.00%> (ø)

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

Impacted Files	Coverage Δ
superset/charts/schemas.py	99.35% <ø> (ø)
superset/connectors/sqla/models.py	77.60% <100.00%> (-13.61%)	⬇️
superset/utils/dashboard_import_export.py	0.00% <0.00%> (-100.00%)	⬇️
superset/tags/core.py	4.54% <0.00%> (-95.46%)	⬇️
superset/key_value/commands/update.py	0.00% <0.00%> (-90.91%)	⬇️
superset/key_value/commands/delete.py	0.00% <0.00%> (-87.88%)	⬇️
superset/key_value/commands/delete_expired.py	0.00% <0.00%> (-84.00%)	⬇️
superset/dashboards/commands/importers/v0.py	15.62% <0.00%> (-76.25%)	⬇️
superset/datasets/commands/create.py	30.61% <0.00%> (-69.39%)	⬇️
superset/datasets/commands/update.py	25.00% <0.00%> (-69.05%)	⬇️
... and 285 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[john-bodley]

@villebro thanks for the context, however that wasn't clear from the description of the original PR. I mostly agree that rolling forward is often easier than rolling back, however I do sense there are a number of shortcomings with the existing logic and having said logic merged—albeit a month ago—shouldn't imply precedence over potential short comings with the implementation.

Granted reverting this feature will likely churn existing deployments, but equally deployments moving forward with this SHA are equally likely going to negatively impact their users. Personally a change like this—simply in nature but potentially profound in terms of impact—should likely be placed behind a feature flag to ensure i) that all the necessary wrinkles can be ironed out, and ii) deployments can coordinate their messaging and/or one off migrations to remedy charts which may be impacted by said change.

[michael-s-molina]

@rusackas @michael-s-molina who might have additional context.

If I'm not mistaken, reverting this PR will break the Drill to Detail feature. If we decide to revert it then we need to fix all the places where it's being used. @rusackas

[ktmud]

I'm supportive of allowing EQUAL and NOT EQUAL to support <NULL>, but we should not allow implicit empty values (None or undefined)---at least on the frontend. The conversion between and "smart" handling of None, NULL, empty string and SQL null are just too complex and error-prone---as demonstrated by the bugs mentioned in this and the other PR. To end users, NULL is a special value and should be clearly represented as such.

[john-bodley]

@michael-s-molina at Airbnb we've going to revert it locally, so it's definitely not overly time critical to revert (if that's what the consensus is) in open source.

[john-bodley]

Note @michael-s-molina, @villebro, and @zhaoyongjie I may be misinformed about what the current situation is regarding how the ad-hoc filters currently handle (or partially handle) string values encoded as <NULL>, i.e., the original PR might be less contraversal than I originally thought.

[zhaoyongjie]

@john-bodley I think the original PR intends to process the None(None in Python, undefined in JS) operand in the QueryObject instead of preparing to make changes in the UX. The major use case is ad-hoc filters in Native Filter, Cross Filter and Drill-to-detail.

Before and after the original PR, the ad-hoc filter modal is not prepared to support None or an empty string to generate a filter field in the QueryObject. When we want to generate is null snippet in Equal to or In operator in ad-hoc modal, the <NULL> operand is designed to do that.

[villebro]

@john-bodley I echo @zhaoyongjie 's comments here that the original PR was not intended to directly make it's way into the UI; rather, the intent was to facilitate drilling/cross filtering, where a user may click on a NULL slice, and is expecting that to trigger a where clause that specifically picks out NULLs.

I agree that

dim_country IN ('<NULL>', 'foo')

should not equate to

"dim_country IS NULL OR dim_county IN ('foo')

, so this should definitely be addressed. If I were to quickly open a PR that fixes this (=makes sure the frontend sends NULL filter events as real nulls, not '<NULL>' string literals, and the backend stops interpreting '<NULL>' string literals as None in the backend), are you ok closing this revert PR?

[rusackas]

Hey @john-bodley et al, is there any reason this should still be considered/revisited, or shall we close it out? Seems like water under the bridge at this point.