refactor: Removes the deprecated GENERIC_CHART_AXES feature flag #26372

michael-s-molina · 2023-12-27T19:26:46Z

SUMMARY

As part of the 4.0 approved initiatives, this PR removes the deprecated GENERIC_CHART_AXES feature flag.

The previous value of the feature flag was True and now the feature is permanently enabled.

TESTING INSTRUCTIONS

CI should be sufficient for merging this PR. We'll do a complete testing of 4.0 after all approved proposals are merged.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

codecov · 2023-12-27T19:33:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d8f7e2c) 67.29% compared to head (bdd6843) 69.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26372      +/-   ##
==========================================
+ Coverage   67.29%   69.65%   +2.35%     
==========================================
  Files        1895     1895              
  Lines       74249    74179      -70     
  Branches     8257     8205      -52     
==========================================
+ Hits        49968    51671    +1703     
+ Misses      22206    20461    -1745     
+ Partials     2075     2047      -28

Flag	Coverage Δ
hive	`53.81% <25.00%> (?)`
mysql	`77.97% <75.00%> (-0.01%)`	⬇️
postgres	`78.07% <75.00%> (-0.01%)`	⬇️
presto	`53.76% <25.00%> (?)`
python	`83.02% <100.00%> (+4.80%)`	⬆️
sqlite	`77.66% <75.00%> (-0.01%)`	⬇️
unit	`56.43% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yousoph · 2024-01-05T22:23:34Z

We have some small improvements/fixes that we are planning to work on to help unblock some of our users from being able to enable the GENERIC_CHART_AXES flag. It would be appreciated if we could hold on merging this PR near the end of the breaking change window for Superset 4.0 so that we can get our changes completed first :) Thanks!!

rusackas · 2024-01-10T21:07:13Z

Cypress Tests LGTM.... I can hit the code owner approval button, but might like to have a more holistic review/approval first.

Vitor-Avila · 2024-01-12T17:18:12Z

hey @michael-s-molina one concern with the GENERIC_CHART_AXES removal, is that currently with the FF set to True the TIME GRAIN control for legacy charts is no longer visible:

Reported in #26474.

Existing charts will still work, but users are no longer able to change the time grain configuration if needed. Also, new legacy charts can't be properly created without this configuration.

Given we're not yet automatically migrating all legacy charts to ECharts, the full removal of the FF would introduce these issues to Organizations still using legacy charts.

I suspect we can fix this by setting the time_grain_sqla's visibility to return true in case we don't have an x_axis set in controls (in superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx). Something like this: https://github.com/apache/superset/compare/master...Vitor-Avila:superset:time-grain-legacy-charts?expand=1

Unsure if we would want to fix the bug first, and then make sure it's working properly before we move forward with this removal PR, or if you would actually prefer to include the fix here. I'm also further testing that branch to confirm if additional changes would be needed.

Let me know your thoughts! Happy to submit a PR from that branch.

michael-s-molina · 2024-01-12T17:30:05Z

Hi @Vitor-Avila. You're correct, we should solve this in a specific PR and merge it before merging this PR. This problem was reported before and fixed for each chart that does not correctly handle both feature flag states. Check #26439 and #23865 for more context.

villebro · 2024-01-12T17:33:14Z

I suggest we introduce two new shared time sections for legacy charts:

one that works with temporal charts (=has time grain)
one that works with non-temporal charts

Essentially these would be the same as genericTime and legacyRegularTime, but without the check for hasGenericChartAxes. These could then be used on legacy charts that don't want to support generic x-axis. These could then be removed once the last legacy chart has been removed.

michael-s-molina · 2024-01-12T17:46:53Z

@villebro @Vitor-Avila Can any of you open a PR to fix this problem? Maybe we can even ask help from @sivasathyaseeelan. I have many things on my plate right now 😢

Vitor-Avila · 2024-01-12T17:57:41Z

I feel like @villebro's suggestion seems more granular and scalable, but I'm still ramping up so I might not have the knowledge to implement it.

I thought this was mostly related with how we're checking if time_grain_sqla should be visible in the chart controls, but seems like it's also related with each visualization type configuration.

villebro · 2024-01-12T18:01:07Z

@Vitor-Avila this should be a fairly straight forward change, and I can support you if you wish. But if you prefer I can open a PR and tag you for a review.

sivasathyaseeelan · 2024-01-12T20:31:40Z

@villebro @Vitor-Avila Can any of you open a PR to fix this problem? Maybe we can even ask help from @sivasathyaseeelan. I have many things on my plate right now 😢

@michael-s-molina I would love to help you if you prefer

Vitor-Avila · 2024-01-15T02:23:54Z

hey @villebro I was checking both the line and bar charts from nvd3 (the charts I reproduced the missing time grain issue), and I believe they both use the legacyTimeseriesTime:

export const legacyTimeseriesTime: ControlPanelSectionConfig = {
  ...baseTimeSection,
  controlSetRows: [
    ['granularity'],
    ['granularity_sqla'],
    ['time_grain_sqla'],
    ['time_range'],
  ],
};

I suspect the issue here is that the visibility settings for time_grain_sqla actually doesn't account for the scenario in which GENERIC_CHART_AXES is enabled but the chart doesn't have an X Axis (a legacy temporal chart):

const time_grain_sqla: SharedControlConfig<'SelectControl'> = {
  type: 'SelectControl',
  label: TIME_FILTER_LABELS.time_grain_sqla,

  ...

  visibility: ({ controls }) => {
    if (!hasGenericChartAxes) {
      return true;
    }

    const xAxis = controls?.x_axis;
    const xAxisValue = xAxis?.value;
    if (isAdhocColumn(xAxisValue)) {
      return true;
    }
    if (isPhysicalColumn(xAxisValue)) {
      return !!(xAxis?.options ?? []).find(
        (col: ColumnMeta) => col?.column_name === xAxisValue,
      )?.is_dttm;
    }
    return false;
  },
};

Is your suggestion to create these two new shared time sections so that legacy nvd3 charts also support X-Axis (categorical) axes? We can also sync tomorrow via Slack.

geido · 2024-01-16T17:04:45Z

superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx

+            name: 'time_grain_sqla',
+            config: {
+              ...sharedControls.time_grain_sqla,
+              visibility: ({ controls }) => {


Not related to this PR. There are bunch of repeated logics between pivot table, chart table, and Box Plot that we could probably centralize at some point

True. I also noticed many repeated code between ECharts plugins. I'm delaying removing this repetition until after the migrations because I think we'll have a better overview of the common patterns.

geido · 2024-01-16T17:05:58Z

superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx

@@ -401,7 +401,7 @@ export default function DateFilterLabel(props: DateFilterControlProps) {
          ref={labelRef}
        />
      </Tooltip>
-      {/* the zIndex value is from trying so that the Modal doesn't overlay the AdhocFilter when GENERIC_CHART_AXES is enabled */}
+      {/* the zIndex value is from trying so that the Modal doesn't overlay the AdhocFilter */}


Unrelated: Not sure I get the meaning of the sentence in this comment

@geido The z-index value comes from trying, I'm not sure what's value is an appreciate value when I made this function.

I think the developer meant that the z-index value was set, after multiple tentative values, to make sure the modal doesn't overlay the filter.

Agree it's confusing 🤷🏼‍♂️

The problem here is that we don't have a z-index registry which would automatically resolve this type of issue. Maybe we'll have one some day.

michael-s-molina · 2024-01-26T21:04:09Z

@mistercrunch It seems the CI step called Label Migration PR is failing. Could you take a look?

michael-s-molina · 2024-01-29T14:29:33Z

/testenv up

github-actions · 2024-01-29T14:31:33Z

@michael-s-molina Ephemeral environment spinning up at http://35.89.141.221:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions · 2024-01-31T14:46:33Z

Ephemeral environment shutdown and build artifacts deleted.

rusackas · 2024-01-31T16:46:44Z

Woohoo!!!!

…che#26372)

michael-s-molina added risk:breaking-change Issues or PRs that will introduce breaking changes hold! On hold v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch labels Dec 27, 2023

pull-request-size bot added the size/XXL label Dec 27, 2023

michael-s-molina marked this pull request as ready for review January 5, 2024 18:52

michael-s-molina requested review from jinghua-qa, geido, eschutho, rusackas, betodealmeida and a team as code owners January 5, 2024 18:52

michael-s-molina force-pushed the remove-generic-chart-axes-ff branch from e64e0f3 to 3b35c65 Compare January 12, 2024 14:02

michael-s-molina removed the hold! On hold label Jan 16, 2024

geido approved these changes Jan 16, 2024

View reviewed changes

Vitor-Avila mentioned this pull request Jan 21, 2024

fix(legacy-charts): Show Time Grain control for legacy charts #26705

Merged

9 tasks

michael-s-molina force-pushed the remove-generic-chart-axes-ff branch from 3b35c65 to e04470c Compare January 22, 2024 12:41

michael-s-molina requested review from kgabryje, justinpark and john-bodley January 22, 2024 13:22

michael-s-molina requested a review from villebro January 22, 2024 13:23

michael-s-molina force-pushed the remove-generic-chart-axes-ff branch from e04470c to 6009828 Compare January 26, 2024 21:01

michael-s-molina added the risk:db-migration PRs that require a DB migration label Jan 26, 2024

michael-s-molina force-pushed the remove-generic-chart-axes-ff branch from 6009828 to d355f1c Compare January 31, 2024 14:04

michael-s-molina and others added 6 commits January 31, 2024 09:20

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag

59b3dfe

Sets PR number

ae8f260

Fixes pylint errors

0f4fdbc

Fixes E2E test

879b3c9

Fixes chart type

de3901e

Update UPDATING.md

bdd6843

michael-s-molina force-pushed the remove-generic-chart-axes-ff branch from d355f1c to bdd6843 Compare January 31, 2024 14:21

michael-s-molina merged commit 8a2f7d3 into apache:master Jan 31, 2024
45 checks passed

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag (apa…

6a53665

…che#26372)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag #26372

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag #26372

michael-s-molina commented Dec 27, 2023 •

edited

Loading

codecov bot commented Dec 27, 2023 •

edited

Loading

yousoph commented Jan 5, 2024

rusackas commented Jan 10, 2024

Vitor-Avila commented Jan 12, 2024 •

edited

Loading

michael-s-molina commented Jan 12, 2024

villebro commented Jan 12, 2024

michael-s-molina commented Jan 12, 2024

Vitor-Avila commented Jan 12, 2024

villebro commented Jan 12, 2024

sivasathyaseeelan commented Jan 12, 2024 •

edited

Loading

Vitor-Avila commented Jan 15, 2024

geido Jan 16, 2024 •

edited

Loading

michael-s-molina Jan 16, 2024

geido Jan 16, 2024

zhaoyongjie Jan 16, 2024

michael-s-molina Jan 16, 2024

michael-s-molina Jan 16, 2024

michael-s-molina commented Jan 26, 2024

michael-s-molina commented Jan 29, 2024

github-actions bot commented Jan 29, 2024

github-actions bot commented Jan 31, 2024

rusackas commented Jan 31, 2024

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag #26372

refactor: Removes the deprecated GENERIC_CHART_AXES feature flag #26372

Conversation

michael-s-molina commented Dec 27, 2023 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 27, 2023 • edited Loading

Codecov Report

yousoph commented Jan 5, 2024

rusackas commented Jan 10, 2024

Vitor-Avila commented Jan 12, 2024 • edited Loading

michael-s-molina commented Jan 12, 2024

villebro commented Jan 12, 2024

michael-s-molina commented Jan 12, 2024

Vitor-Avila commented Jan 12, 2024

villebro commented Jan 12, 2024

sivasathyaseeelan commented Jan 12, 2024 • edited Loading

Vitor-Avila commented Jan 15, 2024

geido Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jan 16, 2024

Choose a reason for hiding this comment

geido Jan 16, 2024

Choose a reason for hiding this comment

zhaoyongjie Jan 16, 2024

Choose a reason for hiding this comment

michael-s-molina Jan 16, 2024

Choose a reason for hiding this comment

michael-s-molina Jan 16, 2024

Choose a reason for hiding this comment

michael-s-molina commented Jan 26, 2024

michael-s-molina commented Jan 29, 2024

github-actions bot commented Jan 29, 2024

github-actions bot commented Jan 31, 2024

rusackas commented Jan 31, 2024

michael-s-molina commented Dec 27, 2023 •

edited

Loading

codecov bot commented Dec 27, 2023 •

edited

Loading

Vitor-Avila commented Jan 12, 2024 •

edited

Loading

sivasathyaseeelan commented Jan 12, 2024 •

edited

Loading

geido Jan 16, 2024 •

edited

Loading