feat: Logic added to limiting factor column in Query model #13521

AAfghahi · 2021-03-08T23:14:20Z

SUMMARY

This is the logic for the migration that can be found here: #14234

What this does is that it assigns one of five categories to each query based on if there is a limiting factor in the results that the query is grabbing. This is in order to create more intelligent error messages in the superset frontend.

Reason

Will be used in the frontend for better user facing messages. Previously several users had complained that they had downloaded the CSV of a query not realizing that the query had been limited either by the dropdown select or their own query parameters. This column will address using backend logic to determine what type of limiting factor, if any, has been placed on the query.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Added unit tests!

ADDITIONAL INFORMATION

Requires DB Migration.

superset/migrations/versions/b5676c717e8b_add_new_columns_to_query_model.py

superset/models/sql_lab.py

superset/sql_lab.py

superset/views/core.py

betodealmeida

Looks good! Did you get a chance to test the migration by running superset db upgrade?

AAfghahi · 2021-03-09T15:04:23Z

Looks good! Did you get a chance to test the migration by running superset db upgrade?

I just did, and it seemed to go through.

(Also happy birthday!)

github-actions · 2021-03-12T21:39:00Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-03-24T18:29:06Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-03-30T20:31:54Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-04-09T03:22:56Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-04-13T18:41:39Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-04-15T14:53:28Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

codecov · 2021-04-15T20:32:42Z

Codecov Report

Merging #13521 (e9d2b3c) into master (9097c29) will decrease coverage by 0.23%.
The diff coverage is 89.74%.

❗ Current head e9d2b3c differs from pull request most recent head 57ceca3. Consider uploading reports for the commit 57ceca3 to get more accurate results

@@            Coverage Diff             @@
##           master   #13521      +/-   ##
==========================================
- Coverage   76.92%   76.68%   -0.24%     
==========================================
  Files         955      955              
  Lines       48055    48080      +25     
  Branches     6033     6034       +1     
==========================================
- Hits        36967    36871      -96     
- Misses      10890    11011     +121     
  Partials      198      198

Flag	Coverage Δ
hive	`?`
javascript	`71.55% <ø> (-0.02%)`	⬇️
mysql	`81.01% <89.74%> (+<0.01%)`	⬆️
postgres	`81.04% <89.74%> (+<0.01%)`	⬆️
presto	`?`
python	`81.13% <89.74%> (-0.44%)`	⬇️
sqlite	`80.65% <89.74%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/reducers/sqlLab.js	`34.95% <ø> (ø)`
superset/sql_lab.py	`81.90% <81.81%> (+0.33%)`	⬆️
superset/views/core.py	`75.70% <86.66%> (+0.05%)`	⬆️
superset/db_engine_specs/base.py	`87.66% <100.00%> (-0.44%)`	⬇️
superset/models/core.py	`88.85% <100.00%> (-0.28%)`	⬇️
superset/models/sql_lab.py	`92.30% <100.00%> (+0.41%)`	⬆️
superset/sql_parse.py	`99.38% <100.00%> (ø)`
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`70.32% <0.00%> (-17.08%)`	⬇️
superset/db_engine_specs/presto.py	`83.36% <0.00%> (-6.53%)`	⬇️
... and 24 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 9097c29...57ceca3. Read the comment docs.

github-actions · 2021-04-15T21:09:02Z

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

superset/views/core.py

betodealmeida · 2021-04-15T21:47:40Z

@AAfghahi do you want to split this PR into migration + limiting_factor detection logic?

For the limiting_factor detecting logic we need to test with all possible cases, and also add some unit tests to make sure we're not returning more data than requested, or less.

AAfghahi · 2021-04-15T22:02:41Z

@betodealmeida that makes sense, so then should I move everything that isn't the migration to another PR?

And then add unit tests to the limiting factor one?

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

superset/views/core.py

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

* master: (38 commits) refactor(native-filters): allow cascading only for filter_select (apache#14441) test(maximize-chart): Add tests to maximize chart action (apache#14371) fix: fixing mysql error message (apache#14416) feat: Logic added to limiting factor column in Query model (apache#13521) change relationship (apache#14435) fix: bootstrap data permissions (apache#14348) fix: parse simple string error message values (apache#14360) chore: add stack trace to all calls of logger.error (apache#14382) update README with new docs and recordings (apache#14432) Renamed impyla from implya in impala.mdx and Renamed PIP package impyla from impala in index.mdx (apache#14425) fix(native-filters): fix filter scope error (apache#14426) feat: Adding limiting_factor column to Query model (apache#14234) feat: Add etag caching to dashboard APIs (apache#14357) chore: Moves Card to the components folder (apache#14139) feat: Dynamic imports for the Icons component (apache#14318) feat: Support env vars configuration for WebSocket server (apache#14398) fix: SQLLab role permissions (apache#14372) fix(native-filters): always show filters without dataset (apache#14409) fix error getting partitionQuery from table.partition (apache#14369) refactor: Boostrap to AntD - Tabs (apache#14048) ...

* Sqllab limit * Add migration script * Set default values * initial push * revisions * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * moving migration to separate PR * with migration * revisions * Fix apply_limit_to_sql * all but tests * added unit tests * revisions * Update superset/sql_lab.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/sql_parse.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * fixed black issue * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * updated logic Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> (cherry picked from commit e507508)

* Sqllab limit * Add migration script * Set default values * initial push * revisions * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * moving migration to separate PR * with migration * revisions * Fix apply_limit_to_sql * all but tests * added unit tests * revisions * Update superset/sql_lab.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/sql_parse.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * fixed black issue * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * updated logic Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

) * Sqllab limit * Add migration script * Set default values * initial push * revisions * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * moving migration to separate PR * with migration * revisions * Fix apply_limit_to_sql * all but tests * added unit tests * revisions * Update superset/sql_lab.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/sql_parse.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * fixed black issue * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * updated logic Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> (cherry picked from commit e507508)

) * Sqllab limit * Add migration script * Set default values * initial push * revisions * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * moving migration to separate PR * with migration * revisions * Fix apply_limit_to_sql * all but tests * added unit tests * revisions * Update superset/sql_lab.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/sql_parse.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * fixed black issue * Update superset/views/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * updated logic Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

pull-request-size bot added the size/L label Mar 8, 2021

AAfghahi changed the title ~~Querydb migration~~ feat: Querydb migration Mar 8, 2021

AAfghahi force-pushed the querydb_migration branch from dc48414 to bfa3b05 Compare March 8, 2021 23:18

betodealmeida self-requested a review March 9, 2021 00:03

betodealmeida reviewed Mar 9, 2021

View reviewed changes

superset/migrations/versions/b5676c717e8b_add_new_columns_to_query_model.py Outdated Show resolved Hide resolved

betodealmeida reviewed Mar 9, 2021

View reviewed changes

superset/models/sql_lab.py Outdated Show resolved Hide resolved

betodealmeida reviewed Mar 9, 2021

View reviewed changes

superset/sql_lab.py Outdated Show resolved Hide resolved

betodealmeida reviewed Mar 9, 2021

View reviewed changes

superset/views/core.py Outdated Show resolved Hide resolved

betodealmeida reviewed Mar 9, 2021

View reviewed changes

betodealmeida added the risk:db-migration PRs that require a DB migration label Mar 9, 2021

AAfghahi force-pushed the querydb_migration branch from 1472ac2 to c902b3e Compare March 10, 2021 23:39

junlincc added the preset-io label Mar 18, 2021

AAfghahi marked this pull request as ready for review April 15, 2021 19:22

AAfghahi requested a review from a team as a code owner April 15, 2021 19:22

AAfghahi force-pushed the querydb_migration branch 4 times, most recently from d120ca4 to aa899de Compare April 15, 2021 20:24

betodealmeida reviewed Apr 15, 2021

View reviewed changes

superset/views/core.py Outdated Show resolved Hide resolved

AAfghahi and others added 7 commits April 30, 2021 10:15

revisions

12e0c46

Fix apply_limit_to_sql

74c6437

all but tests

5175802

added unit tests

6f576b8

revisions

2e8db16

Update superset/sql_lab.py

1e91d21

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

Update superset/sql_parse.py

2f28918

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

AAfghahi force-pushed the querydb_migration branch from 36a4c63 to 9214621 Compare April 30, 2021 14:16

fixed black issue

54bddb2

AAfghahi force-pushed the querydb_migration branch from 9214621 to 54bddb2 Compare April 30, 2021 14:18

betodealmeida reviewed Apr 30, 2021

View reviewed changes

superset/views/core.py Outdated Show resolved Hide resolved

AAfghahi and others added 2 commits April 30, 2021 16:22

Update superset/views/core.py

f528375

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

updated logic

57ceca3

AAfghahi force-pushed the querydb_migration branch from e9d2b3c to 57ceca3 Compare April 30, 2021 20:44

betodealmeida approved these changes Apr 30, 2021

View reviewed changes

betodealmeida merged commit e507508 into apache:master Apr 30, 2021

craig-rueda deleted the querydb_migration branch April 30, 2021 23:45

villebro removed the risk:db-migration PRs that require a DB migration label May 3, 2021

amitmiran137 added the v1.2 label May 4, 2021

mistercrunch added 🍒 1.2.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Logic added to limiting factor column in Query model #13521

feat: Logic added to limiting factor column in Query model #13521

AAfghahi commented Mar 8, 2021 •

edited

Loading

betodealmeida left a comment

AAfghahi commented Mar 9, 2021 •

edited

Loading

github-actions bot commented Mar 12, 2021

github-actions bot commented Mar 24, 2021

github-actions bot commented Mar 30, 2021

github-actions bot commented Apr 9, 2021

github-actions bot commented Apr 13, 2021

github-actions bot commented Apr 15, 2021

codecov bot commented Apr 15, 2021 •

edited

Loading

github-actions bot commented Apr 15, 2021

betodealmeida commented Apr 15, 2021

AAfghahi commented Apr 15, 2021 •

edited

Loading

feat: Logic added to limiting factor column in Query model #13521

feat: Logic added to limiting factor column in Query model #13521

Conversation

AAfghahi commented Mar 8, 2021 • edited Loading

SUMMARY

Reason

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

betodealmeida left a comment

Choose a reason for hiding this comment

AAfghahi commented Mar 9, 2021 • edited Loading

github-actions bot commented Mar 12, 2021

github-actions bot commented Mar 24, 2021

github-actions bot commented Mar 30, 2021

github-actions bot commented Apr 9, 2021

github-actions bot commented Apr 13, 2021

github-actions bot commented Apr 15, 2021

codecov bot commented Apr 15, 2021 • edited Loading

Codecov Report

github-actions bot commented Apr 15, 2021

betodealmeida commented Apr 15, 2021

AAfghahi commented Apr 15, 2021 • edited Loading

AAfghahi commented Mar 8, 2021 •

edited

Loading

AAfghahi commented Mar 9, 2021 •

edited

Loading

codecov bot commented Apr 15, 2021 •

edited

Loading

AAfghahi commented Apr 15, 2021 •

edited

Loading