[GitHub] [superset] villebro commented on pull request #23012: fix: cache warmup solution non legacy charts.

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on PR #23012:
URL: https://github.com/apache/superset/pull/23012#issuecomment-1495578202

   > @villebro per your comment,
   > 
   > > I think a more dynamic approach would be to check if there's a query context for a given slice. If there is, assume it's the new type, and if there isn't, assume it's legacy.
   > 
   > It seems that many legacy charts include the `query_context` and thus this change actually breaks things, i.e., from Airbnb's production data we see:
   > 
   > ```
   > mysql> SELECT DISTINCT viz_type FROM slices WHERE query_context IS NOT NULL ORDER BY viz_type;
   > +----------------------------+
   > | viz_type                   |
   > +----------------------------+
   > | area                       |
   > | bar                        |
   > | big_number                 |
   > | big_number_total           |
   > | box_plot                   |
   > | bubble                     |
   > | cal_heatmap                |
   > | chord                      |
   > | compare                    |
   > | country_map                |
   > | deck_polygon               |
   > | deck_scatter               |
   > | dist_bar                   |
   > | dual_line                  |
   > | echarts_area               |
   > | echarts_timeseries         |
   > | echarts_timeseries_bar     |
   > | echarts_timeseries_line    |
   > | echarts_timeseries_scatter |
   > | echarts_timeseries_smooth  |
   > | echarts_timeseries_step    |
   > | event_flow                 |
   > | filter_box                 |
   > | funnel                     |
   > | gauge_chart                |
   > | graph_chart                |
   > | heatmap                    |
   > | histogram                  |
   > | horizon                    |
   > | line                       |
   > | mapbox                     |
   > | mixed_timeseries           |
   > | para                       |
   > | partition                  |
   > | pie                        |
   > | pivot_table                |
   > | pivot_table_v2             |
   > | radar                      |
   > | sankey                     |
   > | sunburst                   |
   > | table                      |
   > | time_table                 |
   > | treemap                    |
   > | treemap_v2                 |
   > | tree_chart                 |
   > | word_cloud                 |
   > | world_map                  |
   > +----------------------------+
   > 47 rows in set (0.34 sec)
   > ```
   > 
   > Given this is a regression I think that in the interim this should be reverted. Moving forward I wonder if there's a better failsafe mechanism, i.e., maybe try the `ChartDataCommand` route and fallback to the legacy logic if there's an error.
   
   This is curious - legacy charts really shouldn't have a `query_context`, as they don't use it for anything. I wonder if this is caused by first creating a non-legacy chart, running that, then changing chart types. If this is indeed so, we should make sure `query_context` is always set to `null` if the chart doesn't use 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