feat(sql lab): display presto and trino tracking url

[ktmud]

SUMMARY

Enable tracking URL for Presto and Trino (previously it is only available for Hive queries) and also change TRACKING_URL_TRANSFORMER to run at runtime (as opposed to when queries are fetched) so that we can display different tracking URL based on query age (most tracking URLs will expire after a certain amount of time---in Trino/Preso, this is configured by query.min-expire-age.

Had to update query execution logics to add end time for failed executions, too.

While debugging, I also realized the TIMED_OUT status isn't actually in use---and now the frontend also cannot handle that status. We should probably add it back, but let's save it for another PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

When queries are still running:

We will now display the tracking url even for failed jobs, this is useful for users to debug their failed queries:

TESTING INSTRUCTIONS

1. Connect to a Presto or Trino dataset and run some long-running or erroneous queries
2. Optionally, update Superset config to test customize TRACKING_URL_TRANSFORMER. For example, following setting will only display tracking urls for queries finished within the last 5 minutes:

    def TRACKING_URL_TRANSFORMER(url: str, query: "Query") -> Optional[str]:
        if not query.end_time or (
            now_as_float() - float(query.end_time)
            < timedelta(minutes=5).total_seconds() * 1000
        ):
            return re.sub(
                r"^https?://([^/]+)/", "https://trino.datalake.example.org/", url
            )
        return None

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

[codecov]

Codecov Report

Merging #20799 (ee37fbc) into master (1b577d1) will increase coverage by 0.01%.
The diff coverage is 55.88%.

@@            Coverage Diff             @@
##           master   #20799      +/-   ##
==========================================
+ Coverage   66.23%   66.24%   +0.01%     
==========================================
  Files        1757     1757              
  Lines       66978    67031      +53     
  Branches     7117     7118       +1     
==========================================
+ Hits        44360    44408      +48     
- Misses      20801    20806       +5     
  Partials     1817     1817              

Flag	Coverage Δ
hive	53.22% <40.62%> (-0.03%)	⬇️
javascript	51.91% <0.00%> (-0.01%)	⬇️
mysql	81.00% <39.06%> (-0.08%)	⬇️
postgres	81.07% <39.06%> (-0.08%)	⬇️
presto	53.12% <57.81%> (+<0.01%)	⬆️
python	81.55% <59.37%> (+0.01%)	⬆️
sqlite	79.68% <37.50%> (?)
unit	50.23% <32.81%> (-0.04%)	⬇️

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

Impacted Files	Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx	51.38% <0.00%> (-0.36%)	⬇️
superset-frontend/src/components/Button/index.tsx	100.00% <ø> (ø)
superset/sqllab/exceptions.py	67.39% <0.00%> (ø)
superset/utils/dates.py	100.00% <ø> (ø)
superset/views/core.py	75.58% <0.00%> (+0.54%)	⬆️
superset/db_engine_specs/trino.py	73.40% <15.00%> (-15.93%)	⬇️
superset/db_engine_specs/presto.py	87.97% <58.82%> (-1.06%)	⬇️
superset/config.py	91.47% <100.00%> (ø)
superset/db_engine_specs/hive.py	86.87% <100.00%> (+0.99%)	⬆️
superset/models/sql_lab.py	78.90% <100.00%> (+1.52%)	⬆️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

[john-bodley]

Could this just be a method called get_tracking_url and then you wouldn't need to rename the column virtually or have a getter/setter.

[ktmud]

I want the FAB-based API to always return the transformed url but I couldn't find a clean way to do it without adding an (unnecessary) new field.

[ktmud]

@dpgaspar @betodealmeida do you have any recommendation on what's the best practice for such use case?

[ktmud]

info_uri is only available in the latest version of the Trino client, which we haven't upgrade to yet.

[ktmud]

I want the FAB-based API to always return the transformed url but I couldn't find a clean way to do it without adding an (unnecessary) new field.

[ktmud]

Bycatch: wrong import

[ktmud]

@john-bodley I addressed your comments and added some integration tests. Can you take another look?

[john-bodley]

Suggested change

	pass
	return None

[ktmud]

Final return is expected to be explicit if you have a return statement earlier. I think there is a pylint or mypy warning for this.

[john-bodley]

Suggested change

return None

[john-bodley]

See previous comment.

[john-bodley]

@ktmud per you PR description this logic has changed. Can we confirmed that the previous behavior is preserved? I just want to confirm this isn't a breaking change.

[ktmud]

Yes, there is a test case for it: https://github.com/apache/superset/pull/20799/files#diff-bbedc6245e80199586380e684695b5a8e275fd95c46f6770b91eb0bb918e2bf5R52-R53

[ktmud]

Bycatch: this test is flaky. [QUERY_1, QUERY_2, QUERY_3] are executed consequently, with supposedly increasing start_time.

Since the original code int(...) basically rounded down the float epoch, the actual query results returned [QUERY_1, QUERY_2]---even though we filtered second_query_time by QUERY_3. But this still sometimes fail because the rounded down time can be anywhere between QUERY_1 start time and QUERY_3 start time, so if QUERY_1 and QUERY_2 are executed within the same second, we wouldn't have QUERY_2 in the filtered results. E.g. when the start time for the queries are 1.1, 1.5, 2---we'd be filtered to between [1, 1].

[john-bodley]

I would add this at the end of line #420, otherwise I think it disables the error for the entire block.

[ktmud]

I reformatted it so pylint disable isn't necessary.