feat: list view for query history #11574

nytai · 2020-11-05T07:00:52Z

SUMMARY

replaces the Query Search (/superset/sqllab#search) with a new list view
switches the route to /superset/sqllab/history and sets up redirects
missing filters and preview modal (coming in follow up PRs)
behind a feature flag, SIP_34_QUERY_SEARCH_UI until feature complete

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:

After:

TEST PLAN

unit tested
manual testing:
- /superset/sqllab#search -> redirects to /superset/sqllab/search
- Query History page renders without error
- sortable columns all sort without error

ADDITIONAL INFORMATION

Has associated issue:
Changes UI (behind feature flag SIP_34_QUERY_SEARCH_UI)
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

codecov-io · 2020-11-05T19:47:39Z

Codecov Report

Merging #11574 (6020af4) into master (12cb27f) will decrease coverage by 2.84%.
The diff coverage is 18.47%.

@@            Coverage Diff             @@
##           master   #11574      +/-   ##
==========================================
- Coverage   62.87%   60.03%   -2.85%     
==========================================
  Files         887      843      -44     
  Lines       42920    41316    -1604     
  Branches     3989     3722     -267     
==========================================
- Hits        26987    24803    -2184     
- Misses      15754    16348     +594     
+ Partials      179      165      -14

Flag	Coverage Δ
cypress	`54.80% <15.66%> (?)`
javascript	`?`
python	`62.84% <44.44%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/Icon/index.tsx	`100.00% <ø> (ø)`
...rset-frontend/src/components/ListView/ListView.tsx	`77.64% <ø> (-18.36%)`	⬇️
superset-frontend/src/views/App.tsx	`100.00% <ø> (+100.00%)`	⬆️
superset-frontend/src/views/CRUD/data/common.ts	`100.00% <ø> (ø)`
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx	`2.60% <ø> (-58.76%)`	⬇️
superset/config.py	`90.11% <ø> (ø)`
superset/queries/api.py	`100.00% <ø> (ø)`
...t-frontend/src/views/CRUD/data/query/QueryList.tsx	`10.81% <10.81%> (ø)`
superset-frontend/src/SqlLab/components/App.jsx	`60.00% <20.00%> (-1.54%)`	⬇️
superset/views/base.py	`72.68% <33.33%> (-0.51%)`	⬇️
... and 373 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 12cb27f...6020af4. Read the comment docs.

lilykuang

a small nitpick. LGTM

superset-frontend/src/views/CRUD/data/query/QueryList.tsx

etr2460 · 2020-11-10T16:28:18Z

superset-frontend/images/icons/queued.svg

+<rect x="10" y="8" width="8" height="2" rx="1" transform="rotate(90 10 8)" fill="#B2B2B2"/>
+<rect x="13" y="8" width="8" height="2" rx="1" transform="rotate(90 13 8)" fill="#666666"/>
+<rect x="16" y="8" width="8" height="2" rx="1" transform="rotate(90 16 8)" fill="#323232"/>


should these colors be hard coded here?

I don't think we can have multiple colors using the currentColor approach used in other files.

etr2460 · 2020-11-10T16:30:25Z

superset-frontend/spec/javascripts/views/CRUD/annotation/AnnotationList_spec.jsx

@@ -97,15 +97,15 @@ describe('AnnotationList', () => {

  it('fetches annotation layer', () => {
    const callsQ = fetchMock.calls(/annotation_layer\/1/);
-    expect(callsQ).toHaveLength(3);
-    expect(callsQ[2][0]).toMatchInlineSnapshot(
+    expect(callsQ).toHaveLength(2);


why did this change the AnnotationList tests? Seems a bit unexpected to me

It's this line https://github.com/apache/incubator-superset/pull/11574/files#diff-9c9be0048f0d0d4f1bd547faad9f8732fe5a12df6fc185190681914251131cc9R61 that removed an unecessary network request

superset-frontend/src/SqlLab/components/App.jsx

superset-frontend/src/components/Icon/index.tsx

etr2460 · 2020-11-10T16:33:33Z

superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx

@@ -0,0 +1,97 @@
+/**


thanks for the test!

superset-frontend/src/views/CRUD/data/query/QueryList.tsx

etr2460 · 2020-11-10T16:39:07Z

superset-frontend/src/views/CRUD/data/query/QueryList.tsx

+    [],
+  );
+
+  const filters: Filters = useMemo(() => [], []);


why do we need useMemo for this? if it's always a function that returns an array, why not make that a const outside of the render function and them use it in TopAlignedListView?

It's currently an empty array at the moment, but will soon be an array with dynamic values as it is in other list views, eg https://github.com/preset-io/incubator-superset/blob/091432ea8ea4ddd7281eaf41c261f006fe99b352/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx#L319

etr2460 · 2020-11-10T16:39:24Z

superset/config.py

@@ -332,6 +332,7 @@ def _try_json_readsha(  # pylint: disable=unused-argument
    # a custom security config could potentially give access to setting filters on
    # tables that users do not have access to.
    "ROW_LEVEL_SECURITY": False,
+    "SIP_34_QUERY_SEARCH_UI": False,


thanks for using a feature flag!

etr2460 · 2020-11-10T16:40:07Z

superset/views/base.py

+            "common": common_bootstrap_payload(),
+        }
+        return self.render_template(
+            "superset/crud_views.html",


should the base superset view always return the crud_views template? That seems a bit odd to me.

As state below this is repeating another pattern found in other list views. This method is cloned over from SupersetModelView (https://github.com/preset-io/incubator-superset/blob/690689c5caed2becf3a1b6a322fb882df082022a/superset/views/base.py#L338) since inheriting from SupersetModelView in the Superset class was causing an error.

etr2460 · 2020-11-10T16:40:36Z

superset/views/core.py

+        ):
+            return redirect("/superset/sqllab#search", code=307)
+
+        return super().render_app_template()


instead of this being a super function, maybe it should just return a render_crud_views_template that lives in the crud directory?

Sure we could make this a function instead of a method on the base class. I don't see any big advantages there. We've been using this pattern for all other FAB CRUD view replacements, eg https://github.com/preset-io/incubator-superset/blob/091432ea8ea4ddd7281eaf41c261f006fe99b352/superset/views/database/views.py#L104

If this is of concern we can open an issue to clean up how the app templates are rendered for these CRUD view replacements. There are a few other follow up PRs that are waiting for this to be merged so I'm hesitant in adding "refactor all routing/template logic for crud views" to the current scope.

bkyryliuk · 2020-11-11T02:05:56Z

UX / UI question, is there any specific reason why SQL column width is reduced? Our users are looking on sql first while searching for smth & I find it beneficial to keep for space to the query itself

nytai · 2020-11-11T03:51:39Z

@bkyryliuk no specific reason. The designs call for all the other cells to fit on a single line. Your suggestion makes sense though, how does this look?

bkyryliuk · 2020-11-11T16:19:01Z

@bkyryliuk no specific reason. The designs call for all the other cells to fit on a single line. Your suggestion makes sense though, how does this look?

Thats nicer, thanks !

etr2460

lgtm with one other comment (not required to address)

etr2460 · 2020-11-11T17:42:20Z

superset-frontend/src/views/CRUD/data/query/QueryList.tsx

+          const startUtc = new Date(
+            Date.UTC(
+              startDate.getFullYear(),
+              startDate.getMonth(),
+              startDate.getDate(),
+              startDate.getHours(),
+              startDate.getMinutes(),
+              startDate.getSeconds(),
+              startDate.getMilliseconds(),
+            ),
+          );


this feels way too complicated, but maybe this is the only way to do it? Perhaps we can use a library like moment to convert into UTC?

Was the intention to convert the database timestamp to local timestamp? If so, it's obviously not working (at least for my postgres database):

Just ran a query at 11:58pm PT and got 15:58 UTC-8:00 instead.

Since the backend already returns timestamps as UNIX epoch time, you can probably get by without doing special treatment for timezones:

diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx index 220c1a14e..0a223a536 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx @@ -194,20 +194,7 @@ function QueryList({ addDangerToast, addSuccessToast }: QueryListProps) { original: { start_time, end_time }, }, }: any) => { - const startDate = new Date(start_time); - const startUtc = new Date( - Date.UTC( - startDate.getFullYear(), - startDate.getMonth(), - startDate.getDate(), - startDate.getHours(), - startDate.getMinutes(), - startDate.getSeconds(), - startDate.getMilliseconds(), - ), - ); - const startMoment = moment(startUtc); - const formattedStartTimeData = startMoment + const formattedStartTimeData = moment(start_time) .format(DATETIME_WITH_TIME_ZONE) .split(' '); @@ -217,35 +204,18 @@ function QueryList({ addDangerToast, addSuccessToast }: QueryListProps) { {formattedStartTimeData[1]} </> ); - if (!end_time) { - return formattedStartTime; - } - - const endDate = new Date(end_time); - const endUtc = new Date( - Date.UTC( - endDate.getFullYear(), - endDate.getMonth(), - endDate.getDate(), - endDate.getHours(), - endDate.getMinutes(), - endDate.getSeconds(), - endDate.getMilliseconds(), - ), - ); - - return ( + return end_time ? ( <Tooltip title={t( 'Duration: %s', - moment - .utc(moment(endUtc).diff(startMoment)) - .format(TIME_WITH_MS), + moment.utc(end_time - start_time).format(TIME_WITH_MS), )} placement="bottom" > <span>{formattedStartTime}</span> </Tooltip> + ) : ( + formattedStartTime ); }, },

I'm also not sure how useful is the timezone to the users here. Most people would assume it is local time if no timezone is specified.

To be clear this logic currently exists elsewhere in the list views (eg, https://github.com/preset-io/incubator-superset/blob/542d2e3b06095ff90bba382bf0b05adc8e62f91a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx#L290)

The problem, as I understand it, is the these times are being stored without timestamps. So to avoid confusion, we assume the server times are in UTC as is common for most production servers. So any time that is sent from the server without a time zone is assumed to be in utc, so we must parse it as such to display correct results on the client. In your example @ktmud this was done locally where your dev server and db are running on your local timezone. We have had several issues with this before (things looking correct in local but wrong when they're on prod) and I believe this is the agreed upon safest approach.

I did manage to reduce this logic to only use moment though.

cc @graceguo-supercat @riahk @dpgaspar since they're a little more familiar with the problem than I am

As far as time zone info being useful or not, the goal of this PR is to reskin the previous view. Since time zone info existed in the previous view I think we should keep it for now and raise this issue separately where we can achieve consensus on this decision.

The backend returns the timestamp as unix epoch time, which by definition is timezone-less. Maybe different API endpoints handle it differently? @villebro have you done anything in this regard before, too?

either way, it looks like this is working the same way across both versions

Yeah, using moment.utc(..).local(..) is probably safer in case the backend returns ISO timestamp strings without timezones.

@ktmud actually, you are correct. These are unix epochs, which are always in utc (or # seconds from midnight utc...1970). The problem I outlined above only applies to text-based datetimes. I've updated this logic to use your suggestions. Thanks for catching this!

superset-frontend/src/views/CRUD/data/common.ts

ktmud · 2020-11-12T07:58:06Z

It looks like the minimal screen width this view can support is about 1420 (anything less than that would cause a horizontal scroll bar). According to public stats, only about 20% desktop users in the US have a higher resolution than that (probably lower worldwide). Can we use more dynamic column width and make sure this table also look good on smaller screens?

nytai · 2020-11-12T19:29:10Z

@ktmud It does look like the previous version did support a smaller screen width without scroll. I managed to adjust the column widths a bit but was unable to get it to exactly match previous behavior. The previous version was using a smaller font-size (12px vs 14px), smaller header font, and no sorting on the columns, so naturally there was more room available.

All our other list views also suffer from this issue, particularly the ones with lots of columns to display. If this is of concern we should address this issue in the context of all our list views as the goal here is to have unified experience. We could change font-size based on screen width is an idea that immediately comes to mind.

rubypollev · 2021-01-14T17:12:53Z

IMO the check mark / "x" is a downgrade from the pills as an indicator of success state from UX point of view. The larger surface area of pills makes for a more visible status indicator. Now the rows sort of blend in.

superset-github-bot bot added the preset-io label Nov 5, 2020

pull-request-size bot added the size/L label Nov 5, 2020

nytai force-pushed the tai/query-search-listview branch from edd5f5b to 1b8bc5d Compare November 5, 2020 18:47

pull-request-size bot added size/XL and removed size/L labels Nov 5, 2020

nytai force-pushed the tai/query-search-listview branch from e7948f0 to ccc1bbd Compare November 5, 2020 20:10

nytai marked this pull request as ready for review November 6, 2020 05:00

lilykuang approved these changes Nov 9, 2020

View reviewed changes

superset-frontend/src/views/CRUD/data/query/QueryList.tsx Outdated Show resolved Hide resolved

nytai mentioned this pull request Nov 10, 2020

feat: SQL preview modal for Query History #11634

Merged

6 tasks

etr2460 reviewed Nov 10, 2020

View reviewed changes

nytai requested a review from etr2460 November 10, 2020 19:32

etr2460 approved these changes Nov 11, 2020

View reviewed changes

ktmud reviewed Nov 12, 2020

View reviewed changes

superset-frontend/src/views/CRUD/data/common.ts Outdated Show resolved Hide resolved

nytai force-pushed the tai/query-search-listview branch from ddc359e to 3627dda Compare November 12, 2020 18:54

feat: Query History CRUD list view

6020af4

nytai force-pushed the tai/query-search-listview branch from b731870 to 6020af4 Compare November 12, 2020 21:25

nytai merged commit 432e5ab into apache:master Nov 12, 2020

nytai deleted the tai/query-search-listview branch November 12, 2020 21:55

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

feat: Query History CRUD list view (apache#11574)

3322438

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: list view for query history #11574

feat: list view for query history #11574

nytai commented Nov 5, 2020 •

edited

Loading

codecov-io commented Nov 5, 2020 •

edited

Loading

lilykuang left a comment

etr2460 Nov 10, 2020

nytai Nov 10, 2020

etr2460 Nov 10, 2020

nytai Nov 10, 2020

etr2460 Nov 10, 2020

etr2460 Nov 10, 2020

nytai Nov 10, 2020

etr2460 Nov 10, 2020

etr2460 Nov 10, 2020

nytai Nov 10, 2020

etr2460 Nov 10, 2020

nytai Nov 10, 2020

nytai Nov 10, 2020

bkyryliuk commented Nov 11, 2020

nytai commented Nov 11, 2020

bkyryliuk commented Nov 11, 2020

etr2460 left a comment

etr2460 Nov 11, 2020

ktmud Nov 12, 2020

ktmud Nov 12, 2020

nytai Nov 12, 2020

nytai Nov 12, 2020

ktmud Nov 12, 2020

nytai Nov 12, 2020

ktmud Nov 12, 2020

nytai Nov 12, 2020

ktmud commented Nov 12, 2020

nytai commented Nov 12, 2020

rubypollev commented Jan 14, 2021

feat: list view for query history #11574

feat: list view for query history #11574

Conversation

nytai commented Nov 5, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Nov 5, 2020 • edited Loading

Codecov Report

lilykuang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Nov 11, 2020

nytai commented Nov 11, 2020

bkyryliuk commented Nov 11, 2020

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Nov 12, 2020

nytai commented Nov 12, 2020

rubypollev commented Jan 14, 2021

nytai commented Nov 5, 2020 •

edited

Loading

codecov-io commented Nov 5, 2020 •

edited

Loading