Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Partitioned Table UI Enhancements #2110

Merged

Conversation

AkshatJawne
Copy link
Contributor

@AkshatJawne AkshatJawne commented Jun 26, 2024

Resolves #2079 ( Resolves #2066 , Resolves #2103 , Resolves #2104 , Resolves #2105 , Resolves #2106 , Resolves #2107 , Resolves #2108 , Resolves #2109 , Resolves #2049 ), Resolves #2120, Resolves #1904

Changes Implemented:

  • Replaced keysTable with baseTable
  • Made small UI changes to differentiate between Partition Table and Partition-aware source table
  • Allow double clicking to set partition
  • Add context menu action to set partition

Sample Visuals:
Default Partition Table View
Screenshot 2024-06-26 at 2 57 09 PM
Default Partition-aware source table view:
Screenshot 2024-06-26 at 2 56 59 PM

@AkshatJawne AkshatJawne self-assigned this Jun 26, 2024
@AkshatJawne
Copy link
Contributor Author

Looking into unit test failures

@AkshatJawne AkshatJawne marked this pull request as draft June 26, 2024 16:34
@AkshatJawne AkshatJawne marked this pull request as ready for review June 26, 2024 18:14
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGridTableModel.ts Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
@AkshatJawne
Copy link
Contributor Author

BTW, ready for review

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 23.72881% with 90 lines in your changes missing coverage. Please review.

Project coverage is 46.68%. Comparing base (e3aa392) to head (e588409).
Report is 16 commits behind head on main.

Files Patch % Lines
packages/iris-grid/src/IrisGrid.tsx 5.12% 37 Missing ⚠️
...ckages/iris-grid/src/IrisGridPartitionSelector.tsx 52.27% 20 Missing and 1 partial ⚠️
...sehandlers/IrisGridPartitionedTableMouseHandler.ts 17.64% 14 Missing ⚠️
...ges/iris-grid/src/IrisGridPartitionedTableModel.ts 0.00% 10 Missing ⚠️
packages/iris-grid/src/IrisGridTableModel.ts 0.00% 4 Missing ⚠️
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2110      +/-   ##
==========================================
+ Coverage   46.60%   46.68%   +0.08%     
==========================================
  Files         679      691      +12     
  Lines       38408    38587     +179     
  Branches     9687     9622      -65     
==========================================
+ Hits        17901    18016     +115     
- Misses      20455    20560     +105     
+ Partials       52       11      -41     
Flag Coverage Δ
unit 46.68% <23.72%> (+0.08%) ⬆️

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.

packages/iris-grid/src/IrisGridPartitionSelector.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/PartitionedGridModel.ts Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGridTableModel.ts Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGridProxyModel.ts Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
@AkshatJawne AkshatJawne requested a review from mofojed July 8, 2024 18:43
@AkshatJawne
Copy link
Contributor Author

Looking into failures here

@AkshatJawne
Copy link
Contributor Author

Weird that the failures are only occurring on chromium, but I believe the reason of the failure is that we expect to have the pickers accessible in 1.5s, but now, with the new logic, the pickers are disabled while the partitionConfig.mode === 'empty', which in some cases (like the vid below), is not the case.

The reason we do this is so that when we have an empty partition table, we wanted to have the pickers disabled. But even for a table with rows, the table will have 0 rows of length (and thus, will be considered empty) momentarily.

We could either change the benchmark to be 20000ms instead of 15000ms, or we could look at adding a specific "temporarily empty" config mode, which would somehow differ then the "empty" mode (which would now act as more of a forever empty mode).

The latter will be difficult to implement though, since the event listener listens for when the table is updated and then changes the mode accordingly, so I am not sure how before the event, it will differentiate between a table that will be forever empty, and one that is just empty until the data loads in.

@mofojed @dsmmcken any thoughts?

screen-capture.4.webm

@mofojed
Copy link
Member

mofojed commented Jul 9, 2024

We've already got the loading state for when it's still loading. The empty state is for when the table is empty, there's no need for a forever empty state; an empty table could eventually tick in data (it could be in one second, one minute, one hour, we don't really know).
That being said - the failures in the e2e tests are not with the partitioned tables? Where are you seeing that the disabled pickers is affecting the e2e tests? Look at the report attached: https://github.com/deephaven/web-client-ui/actions/runs/9845185336/artifacts/1682155129
Looking at the report it's looking for a context menu that does not appear for some reason.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

I'm going to need a bit more time to think about this one.

.vscode/settings.json Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGrid.tsx Outdated Show resolved Hide resolved
@@ -4226,7 +4258,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
gotoValueSelectedFilter,
partitionConfig,
} = this.state;
if (!isReady) {
if (!isReady || partitionConfig?.mode === 'loading') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to need to think about this a bit more - we should be showing the loading spinner while it's still loading.

Copy link
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

@mofojed, what if we did something like this?

From line 4365-5370 of IrisGrid.tsx, where we could now set the show of the loading text based on loadingSpinnerShown or the configMode === "loading"

Screenshot 2024-07-12 at 8 00 09 AM

@AkshatJawne
Copy link
Contributor Author

No actually, that does not fix the issue we are facing with the spinner, ignore.

@AkshatJawne
Copy link
Contributor Author

Been doing some more investigations, the firefox and cjromium failures here are not consistent. On one run they fail, but then if I re-run those jobs, they pass. The inconsistent behaviour makes me think there may be a race condition or some weird code circumstance in the code responsible for rendering the context menu.

The only change I made to the Context Menu was adding the View Constituent Table option, but I am unsure why that would result in the whole context menu not loading, need to dig more.

@AkshatJawne AkshatJawne force-pushed the 2079_partitioned_table_redesign branch from ee26629 to a092033 Compare July 16, 2024 13:26
@AkshatJawne AkshatJawne requested a review from mofojed July 17, 2024 16:26
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

This is looking good, but the e2e tests are failing: https://github.com/deephaven/web-client-ui/actions/runs/9977913349/job/27578476354?pr=2110
I think it's because of our early return in rebuildFilters() if the filters aren't set, causing it to right-click in the test before the table has appeared.
We can debug in our 1:1, seems kind of tricky

packages/iris-grid/src/IrisGridPartitionedTableModel.ts Outdated Show resolved Hide resolved
@AkshatJawne
Copy link
Contributor Author

AkshatJawne commented Jul 17, 2024

Yeah I have been trying to debug the e2e failures, but it is slightly tricky, will continue to debug and then we can debug together as well in our 1:1, will make the requested change with -1 rn.

@AkshatJawne AkshatJawne merged commit de5ce40 into deephaven:main Jul 17, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.