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

Increase general workbook row limit to 1_048_576 #35484

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

Conversation

gherceg
Copy link
Contributor

@gherceg gherceg commented Dec 5, 2024

Product Description

Technical Summary

Followup to #35426

This is the row limit in Excel and makes more sense than arbitrarily picking 1 million rows. Ideally each feature will have a custom row limit that is appropriate for that workflow, so this is primarily meant as a fallback (as Daniel noted, it isn't really a fallback since this is the Excel limit, and in theory we will never see uploads larger than this...it is basically analogous to setting this limit to None).

Feature Flag

Safety Assurance

Safety story

Marginally increasing a limit. I don't think it is enough to make a meaningful difference to our infrastructure, especially since this limit was just rolled out.

Automated test coverage

QA Plan

No

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@gherceg gherceg marked this pull request as ready for review December 5, 2024 23:12
@@ -1 +1 @@
MAX_WORKBOOK_ROWS = 1_000_000
MAX_WORKBOOK_ROWS = 1_048_576 # max number of rows in Excel
Copy link
Contributor

@millerdev millerdev Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? If it's the limit for Excel then there's no way anyone could submit a file with more rows than this, right?

Or were you trying to put a limiting mechanism in place with the initial limit set as high as possible? Either way, I think it's fine to keep our limit at 1M rows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or were you trying to put a limiting mechanism in place with the initial limit set as high as possible?

Yeah exactly. It felt a bit heavy handed to implement a generic limit that is lower than what it has effectively been without more research/comms, so agree this is effectively making this generic limit moot. If there are workflows where we can support the full 1,048,576 row limit that excel provides, it seems like a better UX to allow that than to arbitrarily limit it to 1 million rows.

This is all under the assumption that we are actively working on implementing specific row limits for features where we have experienced issues in the past, so the problematic areas will be addressed in the near future.

Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I didn't approve the first time.

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.

2 participants