fix(weave_query): prevent table state loss when encountering a new logged column #3060
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=f4e257bebe88c77b771338cd1c8a9a634e3542c8 |
domphan-wandb
force-pushed
the
dom/fix-table-state
branch
4 times, most recently
from
9b32a0d to
1868d0a
Compare
domphan-wandb
force-pushed
the
dom/fix-table-state
branch
from
1868d0a to
51f60ab
Compare
| } | ||
|
|
||
| // 2024/11/20, Note from Dom: |
There was a problem hiding this comment.
this comment could be made more actionable by describing what case we return true here
| if (isList(type) || isList(toType)) { | ||
| if (!isList(type) || !isList(toType)) { |
| if (isTypedDict(type) || isTypedDict(toType)) { | ||
| // Handle union<typedDict> case | ||
| // | ||
| // This is meant for special cases where a user adds another column to their table | ||
| // in a subsequent run. | ||
| if (isTypedDict(toType) && isUnion(type)) { | ||
| return type.members.every( | ||
| member => | ||
| isTypedDict(member) && | ||
| isTypedDict(toType) && | ||
| propertyTypesAreCompatible(member, toType) | ||
| ); | ||
| } |
There was a problem hiding this comment.
everything else is just refactor (?), this lgtm
There was a problem hiding this comment.
Yup, besides the union checking code, I refactored to make it a bit more readable. Hope it didn't make the review hard to parse. Thanks for reviewing my PRs, @gtarpenning , I appreciate it! 🙏
There was a problem hiding this comment.
nice pr! love the thorough tests and extensive write up. this pr was a pleasure to review
my one comment is that we should rename this function from typeShapesMatch to something else, like isConcattable or something (i leave it to you to think up a better name). as it stands, this function is somewhere in between isAssignable() and typeShapesMatch(): it does not check strict type shape equality (the toType can have less columns than the fromType), but it is also not checking assignability (it would return true for string <- number).
Thanks Danny! I'm bad at naming things, so I renamed it to |
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
typeShapesMatchutil function (only used to determine if tableState should be migrated) does not handle the case of atypebeing aunion<typedDict, typedDict>and thetoTypebeing atypedDictwhere the union members match the property types of thetoType'stypedDict.This fix adds a check when the
toTypeis atypedDictand thetypeis aunion<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