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

fix(weave_query): prevent table state loss when encountering a new logged column #3060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

domphan-wandb
Copy link
Contributor

@domphan-wandb domphan-wandb commented Nov 22, 2024

Description

There was a user experience issue causing a table's state (derived columns, sorting, etc) to be wiped when the user logged an additional column with their table in a subsequent run.

Initially I couldn't reproduce it, but realized the issue only occurs after the first new column is added-- subsequent added columns did not reset the table state.

Root cause: the typeShapesMatch util function (only used to determine if tableState should be migrated) does not handle the case of a type being a union<typedDict, typedDict> and the toType being a typedDict where the union members match the property types of the toType's typedDict.

This fix adds a check when the toType is a typedDict and the type is a union<typedDict>. It uses the existing typedDict checking logic to confirm all the members of the union are compatible. I also refactored some of the conditional checks to make it a bit more readable.

I also need to note that the existing behavior did not correctly check the case where two unions are passed in. It will by default return true. In order to reduce the chances of shipping a regression, I've left that untouched and left a comment in the code.

Screen.Recording.2024-11-22.at.11.48.37.AM.mov

Testing

  • Unit tests
  • Local dev QA

@domphan-wandb domphan-wandb requested review from a team as code owners November 22, 2024 16:54
@circle-job-mirror
Copy link

circle-job-mirror bot commented Nov 22, 2024

@domphan-wandb domphan-wandb force-pushed the dom/fix-table-state branch 3 times, most recently from 66bb921 to 9b32a0d Compare November 22, 2024 18:24
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.

1 participant