You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/06/13 14:07:50 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #24388: feat: Allows new values for single value filters
michael-s-molina opened a new pull request, #24388:
URL: https://github.com/apache/superset/pull/24388
### SUMMARY
Allows new values for single value filters. This feature already exists for multiple value filters.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/apache/superset/assets/70410625/3a77d3bb-3859-4d2d-8270-e773b7930a1c
### TESTING INSTRUCTIONS
Check that single value native filters allow free form text.
### ADDITIONAL INFORMATION
- [ ] 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] justinpark commented on a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1228414623
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (!multiSelect && search) {
+ const found = selectOptions.find(option => option.label === search);
Review Comment:
why not matching `option.label` instead of `option.value` ?
--
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] justinpark commented on a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1231323786
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (search && !multiSelect && !hasOption(search, selectOptions, true)) {
+ selectOptions.unshift({
+ label: search,
+ value: search,
+ isNewOption: true,
+ });
+ }
Review Comment:
I personally prefer to have a separate useMemo block for this part since the operation cost of `uniqueOptions` can be high (i.e. airbnb case has > 10k) for processing `uniqWith` and `uniqueOptions.forEach` therefore search value only updated can be inefficient because those (uniqWith, uniqueOptions.forEach) are unnecessary.
```suggestion
const uniqueSelectedOptions = useMemo(() => {
...
return selectOptions;
});
const options = useMemo(() => {
if (search && !multiSelect && !hasOption(search, uniqueSelectedOptions, true)) {
uniqueSelectedOptions.unshift({
label: search,
value: search,
isNewOption: true,
});
}
return uniqueSelectedOptions;
}, [uniqueSelectedOptions, multiSelect, search]);
```
--
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] justinpark commented on a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1231323786
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (search && !multiSelect && !hasOption(search, selectOptions, true)) {
+ selectOptions.unshift({
+ label: search,
+ value: search,
+ isNewOption: true,
+ });
+ }
Review Comment:
I personally prefer to have a separate useMemo block for this part since the operation cost of `uniqueOptions` can be high (i.e. airbnb case has > 10k) so processing `uniqWith` and `uniqueOptions.forEach` when search value only updated can be inefficient.
```suggestion
const uniqueSelectedOptions = useMemo(() => {
...
return selectOptions;
});
const options = useMemo(() => {
if (search && !multiSelect && !hasOption(search, uniqueSelectedOptions, true)) {
uniqueSelectedOptions.unshift({
label: search,
value: search,
isNewOption: true,
});
}
return uniqueSelectedOptions;
}, [uniqueSelectedOptions, multiSelect, search]);
```
--
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] justinpark commented on a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1231323786
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (search && !multiSelect && !hasOption(search, selectOptions, true)) {
+ selectOptions.unshift({
+ label: search,
+ value: search,
+ isNewOption: true,
+ });
+ }
Review Comment:
I personally prefer to have a separate useMemo block for this part since the operation cost of `uniqueOptions` can be high (i.e. airbnb case has > 10k) for processing `uniqWith` and `uniqueOptions.forEach` when search value only updated can be inefficient.
```suggestion
const uniqueSelectedOptions = useMemo(() => {
...
return selectOptions;
});
const options = useMemo(() => {
if (search && !multiSelect && !hasOption(search, uniqueSelectedOptions, true)) {
uniqueSelectedOptions.unshift({
label: search,
value: search,
isNewOption: true,
});
}
return uniqueSelectedOptions;
}, [uniqueSelectedOptions, multiSelect, search]);
```
--
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 merged pull request #24388: feat: Allows new values for single value filters
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #24388:
URL: https://github.com/apache/superset/pull/24388
--
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 a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1228428323
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (!multiSelect && search) {
+ const found = selectOptions.find(option => option.label === search);
Review Comment:
I'll match both
--
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 #24388: feat: Allows new values for single value filters
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24388:
URL: https://github.com/apache/superset/pull/24388#issuecomment-1589524048
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24388](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (416b907) into [master](https://app.codecov.io/gh/apache/superset/commit/a3aacf2527086fac010fdd3f1feb5e9eab3c7562?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a3aacf2) will **decrease** coverage by `1.86%`.
> The diff coverage is `85.18%`.
> :exclamation: Current head 416b907 differs from pull request most recent head 1dde316. Consider uploading reports for the commit 1dde316 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24388 +/- ##
==========================================
- Coverage 69.05% 67.19% -1.86%
==========================================
Files 1903 1903
Lines 74530 74281 -249
Branches 8105 8112 +7
==========================================
- Hits 51464 49914 -1550
- Misses 20955 22248 +1293
- Partials 2111 2119 +8
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| javascript | `55.66% <69.56%> (+0.04%)` | :arrow_up: |
| presto | `?` | |
| unit | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...end/src/components/Datasource/DatasourceEditor.jsx](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YXNvdXJjZS9EYXRhc291cmNlRWRpdG9yLmpzeA==) | `65.35% <ø> (ø)` | |
| [...erset-frontend/src/profile/components/fixtures.tsx](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvY29tcG9uZW50cy9maXh0dXJlcy50c3g=) | `100.00% <ø> (ø)` | |
| [superset/models/sql\_lab.py](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL3NxbF9sYWIucHk=) | `77.51% <0.00%> (-1.07%)` | :arrow_down: |
| [superset/views/base.py](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `77.21% <ø> (ø)` | |
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | `60.71% <41.66%> (-2.11%)` | :arrow_down: |
| [...tend/src/components/Datasource/CollectionTable.tsx](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YXNvdXJjZS9Db2xsZWN0aW9uVGFibGUudHN4) | `82.11% <100.00%> (+0.75%)` | :arrow_up: |
| [...nd/src/dashboard/components/nativeFilters/utils.ts](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvdXRpbHMudHM=) | `72.05% <100.00%> (+1.74%)` | :arrow_up: |
| [superset/config.py](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.95% <100.00%> (+0.02%)` | :arrow_up: |
| [superset/connectors/sqla/models.py](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.58% <100.00%> (-0.05%)` | :arrow_down: |
| [superset/initialization/\_\_init\_\_.py](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvaW5pdGlhbGl6YXRpb24vX19pbml0X18ucHk=) | `89.53% <100.00%> (-1.82%)` | :arrow_down: |
| ... and [3 more](https://app.codecov.io/gh/apache/superset/pull/24388?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [102 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24388/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] justinpark commented on a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1228414623
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (!multiSelect && search) {
+ const found = selectOptions.find(option => option.label === search);
Review Comment:
why matching `option.label` instead of `option.value` ?
--
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 a diff in pull request #24388: feat: Allows new values for single value filters
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24388:
URL: https://github.com/apache/superset/pull/24388#discussion_r1228495201
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -239,16 +236,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const options = useMemo(() => {
const allOptions = [...data];
const uniqueOptions = uniqWith(allOptions, isEqual);
- const selectOptions: { label: string; value: DataRecordValue }[] = [];
+ const selectOptions: SelectOptionsType = [];
uniqueOptions.forEach(row => {
const [value] = groupby.map(col => row[col]);
selectOptions.push({
label: labelFormatter(value, datatype),
value,
});
});
+ if (!multiSelect && search) {
+ const found = selectOptions.find(option => option.label === search);
Review Comment:
Done!
--
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