Skip to content

Conversation

@john-bodley
Copy link
Member

SUMMARY

Not a fix per se to #25995, but an alteration based on #25995 (comment). Per this code block it seems like my hypothesis was correct, i.e., though a cache key is known a priori it's only materialized when the query is successful and thus in our case if the COUNT(*) succeeds but the sample data query fails we should be clearing the cache for the former rather than the later.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

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

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e7797b6) 69.08% compared to head (2a7d92a) 69.10%.
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26060      +/-   ##
==========================================
+ Coverage   69.08%   69.10%   +0.01%     
==========================================
  Files        1941     1940       -1     
  Lines       75892    75869      -23     
  Branches     8443     8443              
==========================================
- Hits        52431    52426       -5     
+ Misses      21286    21268      -18     
  Partials     2175     2175              
Flag Coverage Δ
hive 53.68% <0.00%> (+0.02%) ⬆️
mysql 78.16% <100.00%> (+<0.01%) ⬆️
postgres 78.26% <100.00%> (+<0.01%) ⬆️
presto 53.64% <0.00%> (+0.02%) ⬆️
python 82.95% <100.00%> (+0.03%) ⬆️
sqlite 76.91% <100.00%> (+<0.01%) ⬆️
unit 55.79% <0.00%> (+0.06%) ⬆️

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.

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.

LGTM

@john-bodley john-bodley merged commit bd8951e into apache:master Nov 21, 2023
@john-bodley john-bodley deleted the john-bodley--fix-25995 branch November 21, 2023 21:29
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Nov 22, 2023
michael-s-molina pushed a commit that referenced this pull request Dec 4, 2023
josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🍒 3.0.3 Cherry-picked to 3.0.3 🍒 3.0.4 Cherry-picked to 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 First shipped in 3.1.0 labels Mar 8, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 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/XS v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.3 Cherry-picked to 3.0.3 🍒 3.0.4 Cherry-picked to 3.0.4 🚢 3.1.0 First shipped in 3.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants