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

fix: Removetime_range_endpoints from query context object #19423

Merged
merged 8 commits into from
Mar 31, 2022
Merged

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Mar 30, 2022

SUMMARY

When running reports/alerts charts are getting the following 400 error, due deprecating time_range_endpoints field.

{"message":"Request is incorrect: {'queries': {0: {'extras': {'time_range_endpoints': ['Unknown field.']}}}}"}

To fix this we'll be running a migration to remove all the time_range_endpoints keys from the slice.query_context. We are only updating the query_context that are valid json

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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
Benchmark Results:
Current: 0.42 s
10+: 0.45 s
100+: 0.34 s
1000+: 0.33 s
Cleaning up DB

Downgrade:

INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running downgrade 2ed890b36b94 -> 58df9d617f14, rm_time_range_endpoints_from_qc

Upgrade:

INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 58df9d617f14 -> 2ed890b36b94, rm_time_range_endpoints_from_qc

@hughhhh hughhhh requested a review from a team as a code owner March 30, 2022 01:28
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #19423 (238dceb) into master (8e29ec5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #19423   +/-   ##
=======================================
  Coverage   66.57%   66.57%           
=======================================
  Files        1675     1675           
  Lines       64092    64092           
  Branches     6519     6519           
=======================================
  Hits        42672    42672           
  Misses      19729    19729           
  Partials     1691     1691           
Flag Coverage Δ
hive 52.70% <ø> (ø)
javascript 51.25% <ø> (ø)
mysql 81.91% <ø> (ø)
postgres 81.95% <ø> (ø)
presto 52.55% <ø> (ø)
python 82.39% <ø> (ø)
sqlite 81.72% <ø> (ø)

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


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 8e29ec5...238dceb. Read the comment docs.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

add some codes suggestion.

…nts_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
@hughhhh hughhhh changed the title fix(wip): Removetime_range_endpoints from query context object fix: Removetime_range_endpoints from query context object Mar 30, 2022
…nts_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
@github-actions
Copy link
Contributor

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

Please consider rebasing your branch to avoid db migration conflicts.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

Thanks @hughhhh. LGTM.

@eschutho
Copy link
Member

@hughhhh before merging, I would recommend also running through the db migration benchmark script and checkboxes in the PR description and add the results of the benchmark to the description.

@eschutho
Copy link
Member

This PR is a follow up to #18936

@yousoph
Copy link
Member

yousoph commented Mar 30, 2022

Before this fix, users were getting an error when requesting a CSV or text report. PNG reports were working as expected

Repro steps:

  1. Create a chart of a Table or Pivot Table
  2. Add a time range filter onto your chart
  3. Create a report of your chart

Current behavior:
Report owner will receive an email with a 400 error instead of the report

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, just a suggestion on the try/except.

@hughhhh hughhhh merged commit 129063d into master Mar 31, 2022
@sadpandajoe
Copy link
Member

🏷️ preset:2022.11

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Mar 31, 2022
…9423)

* template for remove time_range_endpoints from query context

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* fix reference

* fix column reference

* fix

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
(cherry picked from commit 129063d)
villebro pushed a commit that referenced this pull request Apr 3, 2022
* template for remove time_range_endpoints from query context

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* fix reference

* fix column reference

* fix

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
(cherry picked from commit 129063d)
@villebro villebro removed the lts-v1 label Apr 4, 2022
@sadpandajoe
Copy link
Member

🏷️ preset:2022.13

@craig-rueda craig-rueda deleted the rm-time-range branch April 14, 2022 21:42
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
…9423)

* template for remove time_range_endpoints from query context

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>

* fix reference

* fix column reference

* fix

* Update superset/migrations/versions/2ed890b36b94_rm_time_range_endpoints_from_qc.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.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 preset:2022.11 preset:2022.13 size/M 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants