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

Data Explorer: Implement partial state eviction when row filters become invalid for pandas, add error messages, invalid filter state, fix row filter bar state synchronization #2827

Merged
merged 14 commits into from
Apr 22, 2024

Conversation

wesm
Copy link
Contributor

@wesm wesm commented Apr 21, 2024

There is some more tire kicking and probably small fixes needed here, but wanted to get this up for review.

General summary:

  • Does fairly rigorous filter invalidation based on schema changes (column deletions, shifts, or type changes) for pandas.
  • Column sort keys are also partially evicted when a column is deleted in the backend -- need to check that this works correctly in the front end.
  • Fixes issue with UI where the row filter buttons disappeared when switching to an editor tab and then back to the data explorer
  • Adds "invalid filter" CSS state (highlighting filter in red) when a filter is invalid
  • Added error_message optional field to RowFilter so that error messages from the backend can be shown in the dropdown modal. Maybe we will want to handle these messages differently, but this at least gives us a start
  • Changes RowFilter.column_index to RowFilter.column_schema. This makes it easier to detect schema changes that affect filters and make them shift or become invalid
  • Switches to ruff when formatting the generated Positron comms
  • Adds "updatedBackendState" event to make it easier to update React states when filtering or schema updates triggers a state synchronization.

Addresses #2584, #2644, #2326

@wesm
Copy link
Contributor Author

wesm commented Apr 21, 2024

I'll add some more videos going through different workflows and write up some code examples

Flow when a filtered column is deleted:

df = pd.DataFrame(
    {"a": [1, 2, 3, 4, 5], "b": ["foo", "bar", None, "baz", "qux"]}
)

# Apply a filter to 'a' and then delete 'a'
del df['a']
Screencast.from.2024-04-21.18-53-39.webm

@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

Filter survives a column permutation

df = pd.DataFrame(
    {"a": [1, 2, 3, 4, 5], "b": ["foo", "bar", None, "baz", "qux"]}
)

# Add a filter on either column and then permute them
df = df[['b', 'a']]
Screencast.from.2024-04-21.19-44-33.webm

@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

This also bounces the filter error from #2759 into the filter modal. I don't want to declare that issue fully resolved because in principal a < 4.5 should work fine for an integer column even though 4.5 does not cast cleanly to integer

image

@wesm wesm requested review from softwarenerd, jmcphers and seeM and removed request for softwarenerd April 22, 2024 00:48
@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

One thing I saw that I would like to fix here is that when a filter is invalidated, and then as a result of a schema update, the invalid filter will work again, that it can be set to valid again. I'll figure that out tomorrow and fix the CI build

@seeM
Copy link
Contributor

seeM commented Apr 22, 2024

Code changes LGTM! Some issues from my testing though:

  1. I tried the scenario of deleting a column that had a filter applied and it doesn't seem to work. The object is updated in the variables pane, and I don't see any errors in the output logs.

    Screen.Recording.2024-04-22.at.08.56.54.mov
  2. Similarly, when I permute the columns, they don't permute in the data explorer view.

    Screen.Recording.2024-04-22.at.09.00.55.mov
  3. When you first apply a filter, the filter flashes red for 1-2 frames before returning to the normal background. This is visible from your first video if you step through it frame-by-frame.

Unrelated to this PR, but I noticed that when you apply a filter, there's no indication of how many rows the table has before versus after filtering. The row + column count on the bottom left only shows after filtering.

@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

I re-ran the two scenarios off the latest just now and it still works for me, maybe your kernel was stale?

Screencast.from.2024-04-22.09-54-09.webm

Thanks for catching the invalid flicker, I just pushed a fix for that

@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

Unrelated to this PR, but I noticed that when you apply a filter, there's no indication of how many rows the table has before versus after filtering. The row + column count on the bottom left only shows after filtering.

Good catch. We may need to return both the filtered and unfiltered shape in the get_state RPC call. I opened #2832

src/vs/workbench/common/theme.ts Outdated Show resolved Hide resolved
@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

I'm done making functional changes now and am just going to kick the tires a bit and make sure nothing else is obviously broken.

@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

Deleting a column that was sorted borks things, so I'm going to try to triage that before this gets merged. EDIT: there is more work here than expected so I'm going to punt this to a follow up PR. Sorting still works fine, just we will need to make some changes to handle state synchronization after a schema update. We also need to fix #2803

@wesm wesm changed the title Implement partial state eviction when row filters become invalid for pandas, add error messages, invalid filter state, fix row filter bar state synchronization Data Explorer: Implement partial state eviction when row filters become invalid for pandas, add error messages, invalid filter state, fix row filter bar state synchronization Apr 22, 2024
@wesm
Copy link
Contributor Author

wesm commented Apr 22, 2024

I was able to fix the sort key state syncing minimally

@wesm wesm merged commit 384e3bd into main Apr 22, 2024
24 checks passed
@wesm wesm deleted the feature/de-schema-change-state-sync branch April 22, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants