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

Move from sheet index -> sheet id #77

Closed
naterush opened this issue Feb 16, 2022 · 3 comments
Closed

Move from sheet index -> sheet id #77

naterush opened this issue Feb 16, 2022 · 3 comments
Labels

Comments

@naterush
Copy link
Member

naterush commented Feb 16, 2022

Is your feature request related to a problem? Please describe.
Currently, we reference sheets by sheet ids. This is nice in-so-far as it allows us to store sheets as an array, and terrible in so far as it complicates a lot of things:

  1. It makes it harder to optimize codes out after deletes: [INVESTIGATION] mitosheet: optimize out unneeded code after deletes #40, because it is hard to track which data frames are what.
  2. It makes it harder to do things like reorder data frames in the sheet.
  3. It makes it harder to remove certain types of metadata from our steps: Snowflake import backend #584.

It also leads to a few bugs that currently exist in the app, that are very hard to fix up otherwise:

  1. Editing pivot tables that have dataframes with lower sheet indexes deleted leads to the params not being gotten properly (aka, you cannot edit them).

Describe the solution you'd like
We'd like to move from sheet indexes -> sheet ids. Doing so requires a few things:

  1. Refactoring the frontend to think about sheet ids, rather than sheet indexes.
  2. Refactoring the steps to take sheet ids, rather than sheet indexes.
  3. Refactoring the apis to take sheet ids, rather than sheet indexes.
  4. Upgrading all previous analyses that take sheet indexes to use sheet ids. Note that this a bit of an undertaking, as we need a good algorithm for building/maintaing sheet ids over time (which, needs some more infrastructure).

All-in-all, I expect this to be a 3-5 day task to complete. It will unlock a considerable amount of new functionality and solutions to problems, and generally seems like something we should have done a long time ago.

Describe alternatives you've considered
I haven't thought much about alternatives. Some form of ID seems necessary, although I guess we could layer it on top of the current system. Hmm.

@naterush
Copy link
Member Author

How to actually move to sheet id: the sheet id should be an incrementing number, that goes up by one during every additional dataframe or every import.

This will allow us to preserve the replaying of analyses properly, while still having a unique ID per dataframe. Weird. It's not exactly an ID.

Or it's like: the IDs need to be consistently applied to the sheet number across repeats of the same analysis.

@naterush
Copy link
Member Author

Oh. I just had an interesting idea:

  1. All passed dataframes have sheet indexes < 0 (counting up to 0).
  2. All imported/created data frames have indexes >= 0 (starting at 0).

This would do some pretty cool things to replaying the analysis, namely where we could avoid issues if users go from not passing a dataframe to passing a dataframe. The analysis would still replay properly. Cool!

@naterush
Copy link
Member Author

naterush commented Mar 20, 2022

NOTE: upgrading to this is a no-go though.. doh. We don't know how many dataframes user pass. We should put this metadata in, methinks!

@naterush naterush closed this as completed Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant