-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: excel sheet upload is not working #10450
Conversation
Thanks @pphszx for the fix! The security manager methods were probably renamed while the original Excel upload PR was under review, hence this error sneaked in. See @10031 for details. Are all the changes here necessary for making this work? If possible, I'd prefer to keep the changes to a minimum, and then follow up with a cleanup PR to add robustness. Not sure if you're aware, but we'll be releasing Ping @blcksrx : as you have most context, you're probably the best person to review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @villebro mentioned above, I'd prefer to keep changes to be minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General suggestion, let's add unit test to prevent regressions like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. I'd also encourage adding a unit test as @bkyryliuk suggested, but understand it may be tricky due to the file format (one would probably need to programmatically generate an Excel file and then try to upload that with different variations of input values, asserting that the resulting table metadata is correct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for cleaning this up! @blcksrx are you ok with these changes?
@villebro Yeah. I'm okay with that |
* remove conflicts with csv upload * revert StringField * change description * remove redundant space * apply string approach
* remove conflicts with csv upload * revert StringField * change description * remove redundant space * apply string approach
* remove conflicts with csv upload * revert StringField * change description * remove redundant space * apply string approach
SUMMARY
The new excel sheet upload feature is not working, this issue is reported in #9825 (comment), this PR fixes it with come changes specific to pandas.read_excel.
e.g. args of chunksize, skipinitialspace are not supported, parse_dates is supported by both, filename arg is io, not filepath_or_buffer, which is for read_csv. sheet_name could be string, but integer should be more common.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION