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 2021/10/06 19:09:23 UTC

[GitHub] [superset] michael-s-molina opened a new pull request #16994: fix: Unnecessary queries when changing filter values

michael-s-molina opened a new pull request #16994:
URL: https://github.com/apache/superset/pull/16994


   ### SUMMARY
   The native filters were firing unnecessary queries when their values changed. This was happening because an `useEffect` hook uses the `formData` as a dependency and the` url_params` property changes every time a value changes. To avoid that, I removed the `url_params` from the comparison but we should definitely improve our `useEffect` dependencies to use more granular information instead of big objects that require deep comparison. Added a `TODO` in the code for future refactoring.
   
   I also changed the dependency in the calculation of the cascade filters tree to avoid unnecessary runs.
   
   @rusackas @villebro @graceguo-supercat 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   https://user-images.githubusercontent.com/70410625/136264904-3860ac19-8dc1-48b4-a017-b2d433587245.mov
   
   https://user-images.githubusercontent.com/70410625/136265001-ce79faec-8e65-46c3-a197-e9ab27e5f2ba.mov
   
   ### TESTING INSTRUCTIONS
   Check the videos for instructions.
   
   ### 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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723932988



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Can we wrap `filterValues` in line 51 in useMemo instead? Like 
   ```
   const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
   ```




-- 
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] AAfghahi commented on pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #16994:
URL: https://github.com/apache/superset/pull/16994#issuecomment-972165259


   🏷 v1.4


-- 
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 merged pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
michael-s-molina merged pull request #16994:
URL: https://github.com/apache/superset/pull/16994


   


-- 
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 #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16994:
URL: https://github.com/apache/superset/pull/16994#issuecomment-936992471


   # [Codecov](https://codecov.io/gh/apache/superset/pull/16994?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 [#16994](https://codecov.io/gh/apache/superset/pull/16994?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (70df9ab) into [master](https://codecov.io/gh/apache/superset/commit/c57719128f5208d83ad15268b3d2bb4d00b62954?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c577191) will **decrease** coverage by `0.00%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/16994/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/16994?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #16994      +/-   ##
   ==========================================
   - Coverage   76.92%   76.92%   -0.01%     
   ==========================================
     Files        1030     1030              
     Lines       55022    55025       +3     
     Branches     7465     7466       +1     
   ==========================================
   + Hits        42328    42329       +1     
   - Misses      12440    12442       +2     
     Partials      254      254              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `70.90% <100.00%> (-0.01%)` | :arrow_down: |
   
   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/16994?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ilters/FilterBar/FilterControls/FilterControls.tsx](https://codecov.io/gh/apache/superset/pull/16994/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2xzLnRzeA==) | `80.00% <ø> (ø)` | |
   | [...veFilters/FilterBar/FilterControls/FilterValue.tsx](https://codecov.io/gh/apache/superset/pull/16994/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlclZhbHVlLnRzeA==) | `68.51% <100.00%> (+0.29%)` | :arrow_up: |
   | [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/16994/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `61.48% <0.00%> (-0.46%)` | :arrow_down: |
   | [...set-frontend/src/views/CRUD/welcome/ChartTable.tsx](https://codecov.io/gh/apache/superset/pull/16994/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9DaGFydFRhYmxlLnRzeA==) | `73.17% <0.00%> (ø)` | |
   | [...frontend/src/views/CRUD/welcome/DashboardTable.tsx](https://codecov.io/gh/apache/superset/pull/16994/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9EYXNoYm9hcmRUYWJsZS50c3g=) | `64.89% <0.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16994?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16994?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c577191...70df9ab](https://codecov.io/gh/apache/superset/pull/16994?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] villebro commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724210396



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       The point of `areObjectsEqual` IMO is to serve as a minimum-effort wrapper around `isEqual` that can be centrally updated if the lodash API were to change (I believe I may have implemented that `ignoreUndefined` some time ago based on some docs I found). I'm all for making this more efficient - if this can be implemented in a simpler fashion let's do it 👍 




-- 
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 a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723977358



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       > Maybe we could enhance `areObjectsEqual` to accept customizer function?
   
   Agreed, let's do that by adding `customizer` to `opts` in `areObjectsEqual`. 




-- 
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 a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724064959



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       > Ah, now I see that `useFilters` hooks returns `Object.entries` which creates a new object on each run... Yeah, that means that it probably needs `JSON.stringify`, but in general I'm not sure if the pattern used in `useFilters` is good
   
   I agree, it's definitely an antipattern. However, until we have a guideline for avoiding them I'd suggest going with `JSON.stringify` for now, leaving a `// TODO:` here and then cleaning these stringifys up in follow-up refactor PRs once we've settled on a good pattern for being able to use strict equality in the deps arrays.




-- 
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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723935527



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       Maybe we could enhance `areObjectsEqual` to accept customizer function?




-- 
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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723932988



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Can we wrap `filterValues` in line 51 in useMemo instead? Like 
   ```
   const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
   ```

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       Maybe we could enhance `areObjectsEqual` to accept customizer function?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Ah, now I see that `useFilters` hooks returns `Object.entries` which creates a new object on each run... Yeah, that means that it probably needs `JSON.stringify`, but in general I'm not sure if the pattern used in `useFilters` is good

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Maybe we could utilise a comparison function in `useSelector` (in `useFilters` hook)? https://react-redux.js.org/api/hooks#useselector

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       However, that might as well be a part of some refactor PR. I'm fine with using `JSON.stringify` here as getting rid of those redundant queries has much higher priority 👍 




-- 
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 change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724111293



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       I'm not sure if I agree. This is the implementation of `areObjectsEqual`:
   
   ```
   export function areObjectsEqual(
     obj1: any,
     obj2: any,
     opts = { ignoreUndefined: false },
   ) {
     let comp1 = obj1;
     let comp2 = obj2;
     if (opts.ignoreUndefined) {
       comp1 = omitBy(obj1, isUndefined);
       comp2 = omitBy(obj2, isUndefined);
     }
     return isEqual(comp1, comp2);
   }
   ```
   
   The first thing is that is very inefficient because it creates new objects (`omitBy`) only for the purpose of removing the properties that are `undefined`. The second thing is that this seems to be a `customizer` implementation and not a new version of `isEqual`. I think the best approach would be to have pre-defined customizers to use with Lodash `isEqualWith`. What do you think?




-- 
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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724063514



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Maybe we could utilise a comparison function in `useSelector` (in `useFilters` hook)? https://react-redux.js.org/api/hooks#useselector




-- 
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 merged pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
michael-s-molina merged pull request #16994:
URL: https://github.com/apache/superset/pull/16994


   


-- 
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 change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724105223



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       I agree. Let's use `JSON.stringify` for now and plan to deal with all occurrences of it in a specific refactor effort. Meanwhile, we should avoid using objects that require deep comparison as `useEffect` dependencies. 

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       I'm not sure if I agree. This is the implementation of `areObjectsEqual`:
   
   ```
   export function areObjectsEqual(
     obj1: any,
     obj2: any,
     opts = { ignoreUndefined: false },
   ) {
     let comp1 = obj1;
     let comp2 = obj2;
     if (opts.ignoreUndefined) {
       comp1 = omitBy(obj1, isUndefined);
       comp2 = omitBy(obj2, isUndefined);
     }
     return isEqual(comp1, comp2);
   }
   ```
   
   The first thing is that is very inefficient because it creates new objects (`omitBy`) only for the purpose of removing the properties that are `undefined`. The second thing is that this seems to be a `customizer` implementation and not a new version of `isEqual`. I think the best approach would be to have pre-defined customizers to use with Lodash `isEqualWith`. What do you think?




-- 
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 a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723976115



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       > Can we wrap `filterValues` in line 51 in useMemo instead? Like
   > 
   > ```
   > const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
   > ```
   
   That's a good idea - however, won't we need to use stringify `filter` in the deps array? Seems like the object reference may change without the contents actually changing.

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       > Maybe we could enhance `areObjectsEqual` to accept customizer function?
   
   Agreed, let's do that by adding `customizer` to `opts` in `areObjectsEqual`. 

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       > Ah, now I see that `useFilters` hooks returns `Object.entries` which creates a new object on each run... Yeah, that means that it probably needs `JSON.stringify`, but in general I'm not sure if the pattern used in `useFilters` is good
   
   I agree, it's definitely an antipattern. However, until we have a guideline for avoiding them I'd suggest going with `JSON.stringify` for now, leaving a `// TODO:` here and then cleaning these stringifys up in follow-up refactor PRs once we've settled on a good pattern for being able to use strict equality in the deps arrays.

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
       time_range,
     });
     const filterOwnState = filter.dataMask?.ownState || {};
+    // TODO: We should try to improve our useEffect hooks to depend more on
+    // granular information instead of big objects that require deep comparison.
+    const customizer = (
+      objValue: Partial<QueryFormData>,
+      othValue: Partial<QueryFormData>,
+      key: string,
+    ) => (key === 'url_params' ? true : undefined);
     if (
       !isRefreshing &&
-      (!areObjectsEqual(formData, newFormData) ||
-        !areObjectsEqual(ownState, filterOwnState) ||
+      (!isEqualWith(formData, newFormData, customizer) ||

Review comment:
       The point of `areObjectsEqual` IMO is to serve as a minimum-effort wrapper around `isEqual` that can be centrally updated if the lodash API were to change (I believe I may have implemented that `ignoreUndefined` some time ago based on some docs I found). I'm all for making this more efficient - if this can be implemented in a simpler fashion let's do it 👍 




-- 
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 change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724105223



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       I agree. Let's use `JSON.stringify` for now and plan to deal with all occurrences of it in a specific refactor effort. Meanwhile, we should avoid using objects that require deep comparison as `useEffect` dependencies. 




-- 
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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724061104



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       Ah, now I see that `useFilters` hooks returns `Object.entries` which creates a new object on each run... Yeah, that means that it probably needs `JSON.stringify`, but in general I'm not sure if the pattern used in `useFilters` is good




-- 
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 a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723976115



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       > Can we wrap `filterValues` in line 51 in useMemo instead? Like
   > 
   > ```
   > const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
   > ```
   
   That's a good idea - however, won't we need to use stringify `filter` in the deps array? Seems like the object reference may change without the contents actually changing.




-- 
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] kgabryje commented on a change in pull request #16994: fix: Unnecessary queries when changing filter values

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r724064415



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
       dataMask: dataMaskSelected[filter.id],
     }));
     return buildCascadeFiltersTree(filtersWithValue);
-  }, [filterValues, dataMaskSelected]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [JSON.stringify(filterValues), dataMaskSelected]);

Review comment:
       However, that might as well be a part of some refactor PR. I'm fine with using `JSON.stringify` here as getting rid of those redundant queries has much higher priority 👍 




-- 
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