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

[INVESTIGATION] mitosheet: optimize out unneeded code after deletes #40

Closed
wants to merge 1 commit into from

Conversation

naterush
Copy link
Member

@naterush naterush commented Feb 3, 2022

Description

This PR implements optimizing out unneeded code after a delete. See the core algorithm for the optimization in the compiler utilities. Generally, pretty happy with how it works, but not the actual implementation (it's very messy). As such, let's not merge this.

What I liked from this implementation:

  1. The general approach of having a set of input and output indexes, that a step performer knows about.
  2. The modification of the step class to be aware of this data (so input and output indexes are wrapped by the step, as well as the newly created indexes makes a lot of sense).

What I didn't like from this implementation:

  1. The lack of symmetry between the return types of input sheet indexes
  2. The existence of a sheet id as being really useful for the algorithms we implemented. It kinda makes me want to... move everything from sheet_index to sheet id. I wonder how this refactor would go!
  3. How our steps manager stores and reports steps. I think the fact that all consumers of the steps in the step manager have to filter them to valid ones is... terrible UX. I want to move to hiding the invalid steps unless you want them; the list of steps should be auto filtered!

Testing

Do some imports, merges, and pivots, and then delete data frames. If the data frame you delete was not used in any later step, it's creation and edits should disappear as well!

Documentation

No. We're also not merging.

@vercel
Copy link

vercel bot commented Feb 3, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/trymito/monorepo/7YKnakmMoSKxJ4cDUCWNdnAcF9LF
✅ Preview: https://monorepo-git-optimize-out-deletes-trymito.vercel.app

@aarondr77
Copy link
Member

aarondr77 commented Feb 6, 2022

Bug: Improperly skipping transpiled code required for duplicate sheet step

Steps to Reproduce

  1. Import a dataframe
  2. Create a pivot table from the dataframe
  3. Duplicate the pivot table
  4. Delete the original pivot table
  5. Notice that the transpiled code removes the creation of the pivot table, but still uses the pivot table in the code.

Screen Shot 2022-02-06 at 5 23 18 PM

Expected Behavior

The code to delete the pivot table should be appended to the end of the transpiled code because the pivot table is required for steps that were not deleted.

@aarondr77
Copy link
Member

This exploration is great. This touches on the primary place that we get feedback about the lack of optimization of the generated code -- still displaying code for deleted dataframes. So I'm glad you started investigating it!

I think just from a semantic perspective, it would be nice to make the move to sheet_ids instead of just creating them in the transpiled step cause it would be helpful in other places as well (ie: we currently have some small bugs that occur from using sheet indexes) But obviously that’s a bigger move.

@aarondr77
Copy link
Member

Doing the sheet_id refactor will also be nice so that we could keep the tabs of graphs and dataframes in the same bar.

@aarondr77 aarondr77 closed this Feb 13, 2022
@marthacryan marthacryan deleted the optimize-out-deletes branch March 14, 2024 20:43
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.

None yet

2 participants