Allow users to estimate query cost before executing it by betodealmeida · Pull Request #8172 · apache/superset

betodealmeida · 2019-09-04T16:03:54Z

SUMMARY

We currently added to Presto support for estimating the number of bytes scanned (trinodb/trino#806), and we'd like to surface that information to SQL Lab users before they actually run a query.

This PR extends the DB specs with an allows_cost_estimate attribute and associated methods, allowing pre-execution costs to be computed from DBs that support it.

In order to use it, the feature flag ESTIMATE_QUERY_COST must be enabled, and it needs to be explicitly turned on for each database that supports query cost estimation. When all those conditions are met, a new button will show up in SQL Lab, allowing users to run cost estimates for the whole query or for the selected SQL.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

DBs where the feature is not supported or not enabled are unmodified:

Here's a Presto DB:

Waiting for results:

The result:

And how errors (timeout, syntax errors) are surfaced:

TEST PLAN

Tested with a Presto cluster that supports query cost estimation, running version 0.319 and with the feature enabled via extra:

{
    "version": "0.319",
    "cost_estimate_enabled": true,
    "metadata_params": {},
    "engine_params": {
       "connect_args": {
           "protocol":"https", 
           "source":"superset"
        }
    },
    "metadata_cache_timeout": {"schema_cache_timeout": 86400, "table_cache_timeout": 86400},
    "default_schemas": ["core", "default"]
}

Additionally, I tested:

Lowering the Presto version removes the button.
Other DBs don't show the button.
Turning off the feature flag removes the button.

ADDITIONAL INFORMATION

REVIEWERS

@etr2460 @mistercrunch

etr2460

a bunch of comments, also @john-bodley or @villebro should probably double check the db engine specs stuff

superset/assets/src/SqlLab/components/EstimateQueryCostButton.jsx

superset/db_engine_specs/base.py

superset/db_engine_specs/presto.py

superset/migrations/versions/8786d6374caa_add_column_for_query_estimate.py

superset/models/core.py

superset/sql_validators/presto_db.py

superset/views/core.py

betodealmeida · 2019-09-10T22:36:15Z

@etr2460, I addressed all the comments. I removed the DB migration, and the feature is enabled per DB in extras. Also, I use the DB version to determine if it's supported.

etr2460 · 2019-09-12T21:54:26Z

superset/db_engine_specs/presto.py

+
+            prefixes = ["K", "M", "G", "T", "P", "E", "Z", "Y"]
+            prefix = ""
+            to_next_prefix = 1000


commenting again, shouldn't this be 1024? And we should make it a const

Sorry, I replied to your comment but I think I resolved the conversation. This is used not just for bytes, but also for cpu and network cost, so 1000 is the correct unit. Also, 1000 is the correct unit for the prefixes K, M, G, etc. For 1024 the prefixes are Ki, Mi, Gi.

Eg, 1024 B = 1 KiB = 1.024 KB.

I had the same concern as @etr2460 , learned something new here (had to google to double check) 👍

etr2460 · 2019-09-12T21:55:42Z

superset/db_engine_specs/presto.py

+
+        db_engine_spec.execute(cursor, sql)
+        polled = cursor.poll()
+        while polled:


Is this the only way to tell if the query is finished? This seems a little sketchy, can we not pass a callback or something on success?

Yeah, let me simplify this.

etr2460 · 2019-09-12T21:55:59Z

superset/db_engine_specs/presto.py

+        result = json.loads(first)
+        estimate = result["estimate"]
+
+        def humanize(value, suffix):


can we add types here?

etr2460 · 2019-09-12T21:56:14Z

superset/models/core.py

        return self.db_engine_spec.allows_subqueries

+    @property
+    def allows_cost_estimate(self):


add a return type?

etr2460 · 2019-09-12T21:56:29Z

superset/views/core.py

+    @expose("/estimate_query_cost/<database_id>/", methods=["POST"])
+    @expose("/estimate_query_cost/<database_id>/<schema>/", methods=["POST"])
+    @event_logger.log_this
+    def estimate_query_cost(self, database_id, schema=None):


etr2460 · 2019-09-12T21:56:47Z

superset/db_engine_specs/base.py

    try_remove_schema_from_table_name = True

+    @classmethod
+    def get_allow_cost_estimate(cls, version=None):


betodealmeida · 2019-09-13T05:39:03Z

@etr2460, I added types and cleaned up the query execution.

etr2460

Would you mind making the test plan a little more robust? Test with the feature flag both enabled and disabled, with presto dbs that are configured at a passing version and prior version? With non presto dbs? I'm sure you've tested other cases, but right now the test plan only references the happy path, so a bit more detail would be great.

other than that and my 2 comments here, this lgtm! I'll approve to unblock

superset/views/core.py

betodealmeida added 4 commits August 30, 2019 16:19

WIP

29a376b

Basic functionality working

d7ff8e0

Enable per DB

465d054

Show error message

e3a3680

pull-request-size bot added the size/L label Sep 4, 2019

Small improvements

7428efc

betodealmeida changed the title ~~Allow users to estimate query cost before executing them~~ Allow users to estimate query cost before executing it Sep 4, 2019

betodealmeida added enhancement:request Enhancement request submitted by anyone from the community .database sqllab Namespace | Anything related to the SQL Lab lyft Related to Lyft risk:db-migration PRs that require a DB migration labels Sep 4, 2019

Fix lint

70c09e9

betodealmeida added the review label Sep 4, 2019

etr2460 requested changes Sep 9, 2019

View reviewed changes

Address comments

d80e6be

etr2460 reviewed Sep 12, 2019

View reviewed changes

Address comments

4ec832d

Remove logging, fix exception

14e23c7

etr2460 approved these changes Sep 16, 2019

View reviewed changes

superset/views/core.py Show resolved Hide resolved

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

betodealmeida added 3 commits September 17, 2019 08:45

Fix tests

5e3118a

Bump FAB requirements

dbe09b0

Merge branch 'master' into VIZ-726

5566ef8

betodealmeida merged commit 8847e10 into apache:master Sep 17, 2019

betodealmeida mentioned this pull request Oct 29, 2019

Allow user to customize query cost estimate #8470

Merged

12 tasks

villebro mentioned this pull request Feb 14, 2022

feat(bigquery): add support for query cost estimate #18694

Closed

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 First shipped in 0.35.0 labels Feb 28, 2024

Comments

Conversation

betodealmeida commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

betodealmeida commented Sep 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betodealmeida commented Sep 13, 2019

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

betodealmeida commented Sep 4, 2019 •

edited

Loading