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/04/05 23:45:25 UTC

[GitHub] [superset] diegomedina248 opened a new pull request, #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

diegomedina248 opened a new pull request, #19540:
URL: https://github.com/apache/superset/pull/19540

   ### SUMMARY
   When the data cache is configured, after a cache invalidation in the explore, the results view is not impacted.
   So, if we modify the underlying dataset while using the explore, and we try to invalidate the cache, then the chart correctly displays the new data, but the view results doesn't.
   
   This PR ensures that, after a cache invalidation, both the chart and results reflect the latest data.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Before:
   
   https://user-images.githubusercontent.com/17252075/161868686-e21df3d3-715b-4dab-8ab4-56bb0c8f928c.mov
   
   After:
   
   https://user-images.githubusercontent.com/17252075/161868702-d739e8b9-4687-4d9c-9bed-b10360554a9b.mov
   
   
   ### TESTING INSTRUCTIONS
   1. Create a table chart with raw records, pointing to a dataset that is easy to update (Gsheets for example)
   2. Add or remove some rows from your dataset
   3. Refresh the cache on the chart
   
   Ensure that, after cache invalidation, both the chart and data results view reflect the latest data.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [x] 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] diegomedina248 commented on a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r863069949


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   @zhaoyongjie looks good to me! feel free to close this PR once yours lands here. Thanks



-- 
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 #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

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

   # [Codecov](https://codecov.io/gh/apache/superset/pull/19540?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 [#19540](https://codecov.io/gh/apache/superset/pull/19540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ec83952) into [master](https://codecov.io/gh/apache/superset/commit/1b4d8ddf7103f3fa18683a0f23dff6d532ad7efa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1b4d8dd) will **increase** coverage by `0.00%`.
   > The diff coverage is `80.00%`.
   
   > :exclamation: Current head ec83952 differs from pull request most recent head edf9773. Consider uploading reports for the commit edf9773 to get more accurate results
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #19540   +/-   ##
   =======================================
     Coverage   66.59%   66.59%           
   =======================================
     Files        1682     1682           
     Lines       64314    64319    +5     
     Branches     6559     6559           
   =======================================
   + Hits        42832    42836    +4     
   - Misses      19781    19782    +1     
     Partials     1701     1701           
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.36% <80.00%> (+<0.01%)` | :arrow_up: |
   
   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/19540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...rset-frontend/src/components/Chart/chartReducer.ts](https://codecov.io/gh/apache/superset/pull/19540/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvY2hhcnRSZWR1Y2VyLnRz) | `25.00% <ø> (ø)` | |
   | [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/superset/pull/19540/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `73.61% <ø> (ø)` | |
   | [...t-frontend/src/explore/reducers/getInitialState.ts](https://codecov.io/gh/apache/superset/pull/19540/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLnRz) | `0.00% <ø> (ø)` | |
   | [...erset-frontend/src/components/Chart/chartAction.js](https://codecov.io/gh/apache/superset/pull/19540/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvY2hhcnRBY3Rpb24uanM=) | `53.74% <71.42%> (-0.06%)` | :arrow_down: |
   | [...nd/src/explore/components/DataTablesPane/index.tsx](https://codecov.io/gh/apache/superset/pull/19540/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVzUGFuZS9pbmRleC50c3g=) | `74.48% <100.00%> (+0.80%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19540?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/19540?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 [1b4d8dd...edf9773](https://codecov.io/gh/apache/superset/pull/19540?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] zhaoyongjie commented on a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r863306177


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   @rusackas @diegomedina248 I will open a new PR today. Thanks.



-- 
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 a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r861467403


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   @diegomedina248 I pushed a [branch](https://github.com/zhaoyongjie/incubator-superset/commit/ffdba0532977ca1663f27a2aa012fed7c82220f8) in my repository to fix this issue. discussion welcome.



-- 
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] diegomedina248 commented on a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r843806426


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   As far as I can tell, that force is used for something else, which is tracked in the `ExploreViewContainer`.
   I also cannot find where in the `exploreReducer` that is being modified, but updating that value there will have other consequences 



-- 
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 a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r843575394


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   I remember that the `force` field is in the explore state, but I can not get a `True` value even though I clicked the `cached` button. Could we resolve this issue through change the `force` value?
   
   ![image](https://user-images.githubusercontent.com/2016594/161919457-d7aac73c-c51f-4c15-a843-395296291c26.png)
   



-- 
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] rusackas commented on pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #19540:
URL: https://github.com/apache/superset/pull/19540#issuecomment-1116418047

   Closing in favor of https://github.com/apache/superset/pull/19932


-- 
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 a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r863336924


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   opened PR at https://github.com/apache/superset/pull/19932.



-- 
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] rusackas closed pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
rusackas closed pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache
URL: https://github.com/apache/superset/pull/19540


-- 
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] rusackas commented on a diff in pull request #19540: fix: View Results Doesn't Match Chart After Refreshing Cache

Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #19540:
URL: https://github.com/apache/superset/pull/19540#discussion_r863177725


##########
superset-frontend/src/explore/components/DataTablesPane/index.tsx:
##########
@@ -242,9 +255,13 @@ export const DataTablesPane = ({
         ...prevIsLoading,
         [resultType]: true,
       }));
+
+      chartUpdateCacheInvalidationStatus(false);
+
       return getChartDataRequest({
         formData: queryFormData,
         resultFormat: 'json',
+        force: !!latestQueryCacheInvalidatedRef.current,
         resultType,
         ownState,
       })

Review Comment:
   @zhaoyongjie were you indeed opening a PR to resolve this, or just providing some advice/examples?



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