You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/08 18:24:51 UTC
[GitHub] [superset] john-bodley opened a new pull request, #22065: Revert "feat: support None operand in EQUAL operator (#21713)"
john-bodley opened a new pull request, #22065:
URL: https://github.com/apache/superset/pull/22065
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
This PR reverts https://github.com/apache/superset/pull/21713 —via `git revert`. @zhaoyongjie and @villebro I love the essence of this PR—which is somewhat akin to what Tableau does—making SQL more attainable especially it it relates to handling `NULL`, sadly I don't believe the change is ready for primetime. Specifically:
1. Aspects of https://github.com/apache/superset/pull/21713#pullrequestreview-1132542047 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).
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
<img width="349" alt="Screen Shot 2022-11-08 at 10 09 56 AM" src="https://user-images.githubusercontent.com/4567245/200644540-e6531d07-0abb-4dc4-8dcf-fd9ba8b95fd0.png">
##### Executed SQL
<img width="229" alt="Screen Shot 2022-11-08 at 10 10 43 AM" src="https://user-images.githubusercontent.com/4567245/200644537-ddad0a54-c3a9-4678-bd4f-e7db8cc073cf.png">
### SIMPLE vs CUSTOM SQL
##### SIMPLE
<img width="345" alt="Screen Shot 2022-11-08 at 10 06 02 AM" src="https://user-images.githubusercontent.com/4567245/200644616-61aa15c4-db1f-4700-9b6d-e7fb9b416d30.png">
##### CUSTOM SQL
<img width="348" alt="Screen Shot 2022-11-08 at 10 06 09 AM" src="https://user-images.githubusercontent.com/4567245/200644606-b01e59b1-8698-49f4-9042-dc9d99cb04fb.png">
##### Executed SQL
<img width="299" alt="Screen Shot 2022-11-08 at 10 06 55 AM" src="https://user-images.githubusercontent.com/4567245/200644599-03b3b332-e39c-4147-af02-44cc6d595a83.png">
### TESTING INSTRUCTIONS
CI.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #22065: Revert "feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307753762
@villebro thanks for the context, however that's not clear from the description of the original PR. I mostly agree that rolling forward is often easier than rolling back, 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 (though simply in nature) though potentially profound in terms of impact should likely be placed behind a feature flag to ensure i) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] revert: feat: support None operand in EQUAL operator (#21713)" [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley closed pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
URL: https://github.com/apache/superset/pull/22065
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #22065: Revert "feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307735745
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1308021107
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>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307780947
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] revert: feat: support None operand in EQUAL operator (#21713)" [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1930637164
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1311488601
@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
```sql
dim_country IN ('<NULL>', 'foo')
```
should not equate to
```sql
"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 `null`s, not `'<NULL>'` string literals, and the backend stops interpreting `'<NULL>'` string literals as `None` in the backend), are you ok closing this revert PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307819200
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307784255
I'm also curious to hear more details about how Native Filter was misbehaving.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22065: Revert "feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1307744194
# [Codecov](https://codecov.io/gh/apache/superset/pull/22065?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22065](https://codecov.io/gh/apache/superset/pull/22065?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (32b1de1) into [master](https://codecov.io/gh/apache/superset/commit/cd1b379bdf323f78c2e7d574525a55898c920942?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cd1b379) will **decrease** coverage by `11.34%`.
> The diff coverage is `100.00%`.
```diff
@@ 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%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.20% <0.00%> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22065?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/charts/schemas.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY2hhcnRzL3NjaGVtYXMucHk=) | `99.35% <ø> (ø)` | |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `77.60% <100.00%> (-13.61%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.61% <0.00%> (-69.39%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| ... and [285 more](https://codecov.io/gh/apache/superset/pull/22065/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #22065: revert: feat: support None operand in EQUAL operator (#21713)"
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #22065:
URL: https://github.com/apache/superset/pull/22065#issuecomment-1308122857
@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 filter 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` string 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org