Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(explore): Migrate BigNumber to v1 api [ID-28][ID-55] #17587

Merged
merged 28 commits into from
Dec 15, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Nov 30, 2021

SUMMARY

This PR's goal is to make the legacy Big Number chart plugin not-so-legacy-anymore.

First improvement is migrating the chart endpoint from legacy to V1 API. This change will also allow proper handling of timestamp format metrics. The legacy API assumed that metric result is always a number - if you used a metric that should return a datetime (e.g. MAX(temporal_column)) with a db that returns dates as strings (like sqlite or druid), you'd get nulls in response (see screenshots).

Second thing - to render the trendline, we used @data-ui/xy-chart library, which hasn't been maintained for 2 years. I replaced it with Echarts, which is our go-to viz library.

Lastly, the legacy plugin wasn't able to handle rolling aggregates properly if values were missing. The new version ensures that rolling windows are resampled with null values when a time grain is used to ensure that the calculation window always has the correct number of lagged values applied.

DB migration benchmark results:

Current: 0.42 s
10+: 0.33 s
100+: 0.30 s
1000+: 0.30 s
10000+: 0.42 s

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

image

After:

image

TESTING INSTRUCTIONS

Big Number and Big Number with Trendline core functionalities should work like before.
Timestamp metrics should be formatted automatically.

Steps to reproduce bug shown on screenshots:

  1. Create a BigNumber or Big Number with Trendline chart using SQLite or Druid datasource
  2. Create a metric that should return a date (like MAX(temporal_column))
  3. Verify that Big Number and Trendline are rendered properly, and that big number and trendline's tooltip are formatted as a date

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

CC @junlincc @jinghua-qa

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #17587 (e674657) into master (07bbe84) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e674657 differs from pull request most recent head 944ae17. Consider uploading reports for the commit 944ae17 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #17587   +/-   ##
=======================================
  Coverage   67.72%   67.72%           
=======================================
  Files        1602     1602           
  Lines       64146    64154    +8     
  Branches     6773     6773           
=======================================
+ Hits        43442    43450    +8     
  Misses      18851    18851           
  Partials     1853     1853           
Flag Coverage Δ
hive 81.78% <100.00%> (+<0.01%) ⬆️
mysql 82.15% <100.00%> (+<0.01%) ⬆️
postgres 82.21% <100.00%> (+<0.01%) ⬆️
python 82.55% <100.00%> (+<0.01%) ⬆️
sqlite 81.89% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...-frontend/src/visualizations/presets/MainPreset.js 100.00% <ø> (ø)
superset/charts/schemas.py 99.34% <100.00%> (+<0.01%) ⬆️
superset/common/query_context_processor.py 91.38% <100.00%> (+0.08%) ⬆️
superset/models/helpers.py 90.66% <100.00%> (+0.07%) ⬆️
...hart-echarts/src/BigNumber/BigNumberTotal/index.ts
...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx
...arts/src/BigNumber/BigNumberWithTrendline/index.ts
...ugin-chart-echarts/src/BigNumber/sharedControls.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07bbe84...944ae17. Read the comment docs.

@kgabryje kgabryje changed the title [WIP] chore(explore): Migrate BigNumber to v1 api chore(explore): Migrate BigNumber to v1 api Nov 30, 2021
@kgabryje kgabryje changed the title chore(explore): Migrate BigNumber to v1 api chore(explore): Migrate BigNumber to v1 api [ID-28][ID-55] Nov 30, 2021
@kgabryje
Copy link
Member Author

kgabryje commented Dec 1, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2021

@kgabryje Ephemeral environment spinning up at http://54.218.60.103:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nit, other than that LGTM and works and looks really great!

Comment on lines +79 to +80
from_dttm: number | null;
to_dttm: number | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears the ChartDataResponseResult schema is pretty badly out of sync with this. For example, the following appear to be missing or have an incorrect name:

  • here we have cache_dttm, on ChartDataResponseResult we have cached_dttm
  • colnames and coltypes are missing on ChartDataResponseResult.
  • from_dttm and to_dttm should also be added to ChartDataResponseResult.
    Let's try to make sure these are in sync.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I think we may need a db migration script

},
},
],
[
{
name: 'header_timestamp_format',
name: 'force_timestamp_formatting',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we renaming existing config props in use? Might need a db migration script if that's the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - tested to be compatible with the old metadata in all cases that I was able to come up with.

@kgabryje kgabryje requested a review from a team as a code owner December 13, 2021 13:26
Comment on lines 57 to 62
header_format_selector = params.pop("header_format_selector", None)
header_timestamp_format = params.pop("header_timestamp_format", None)
if header_format_selector:
params["time_format"] = header_format_selector
if header_timestamp_format:
params["force_timestamp_formatting"] = header_timestamp_format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are inverted - shouldn't header_format_selector be migrated to force_timestamp_formatting and header_timestamp_format to time_format?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting that!

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Retested with migrations and before and after look the same now with forced timestamp on BigNumberTotal.

Comment on lines 87 to 89
except Exception as e:
print(e)
print(f"Parsing params for slice {slc.id} failed.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen a couple migrations that succeeded even when exceptions were raised, and they always had to be fixed later to address inconsistent states.

Here, it would be better to raise the exception, so that the migration fails and the admin has the chance to fix the offending charts before trying again. Otherwise, if the migration succeeds they might not even see the message, leaving their system inconsistent, and even if they see the message they need to downgrade and upgrade, which is more work.

Also, can you update your summary confirming that migrations are atomic, have been tested, and share the benchmark numbers? You can use scripts/benchmark_migration.py to get the numbers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment! I updated the description and and re-raised the exception

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing the migration!

@github-actions
Copy link
Contributor

⚠️ @kgabryje Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

1 similar comment
@github-actions
Copy link
Contributor

⚠️ @kgabryje Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@kgabryje kgabryje merged commit 124af4c into apache:master Dec 15, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@AAfghahi
Copy link
Member

Hello! I am pretty sure this is currently breaking Charts and Dashboards for me. When I try to open a dashboard I get:

image

It fails here: https://github.com/apache/superset/blob/master/superset-frontend/src/visualizations/presets/MainPreset.js#L98

When I comment this out it continues working, but obviously then there are no Big Numbers.

shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
)

* chore(explore): Migrate BigNumber to v1 api

* Move to echarts

* Use Echarts trendline

* Fix imports

* Fix parsing dates as strings

* Add from_dttm and to_dttm to v1 chart response

* Fix post processing

* Fix timeRangeFixed

* Fix tests

* Remove from and to dttm from cache

* Cleanup date formatting

* Fix storybook

* Fix missing types

* Fix timestamp with timezone

* Add types to demo's tsconfig

* bug fix

* fix import

* Fix cypress tests

* add sort

* add resample to handle missing values properly

* Sync ChartDataResponseResult schema with ts interface

* Lint fix

* Add migration

* Fix migration

* Remove pass

* Re-raise the exception in migration

* Typo fix

* Update revision

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
)

* chore(explore): Migrate BigNumber to v1 api

* Move to echarts

* Use Echarts trendline

* Fix imports

* Fix parsing dates as strings

* Add from_dttm and to_dttm to v1 chart response

* Fix post processing

* Fix timeRangeFixed

* Fix tests

* Remove from and to dttm from cache

* Cleanup date formatting

* Fix storybook

* Fix missing types

* Fix timestamp with timezone

* Add types to demo's tsconfig

* bug fix

* fix import

* Fix cypress tests

* add sort

* add resample to handle missing values properly

* Sync ChartDataResponseResult schema with ts interface

* Lint fix

* Add migration

* Fix migration

* Remove pass

* Re-raise the exception in migration

* Typo fix

* Update revision

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants