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

fix: bulk batching wide fIles (cli/#1460) #285

Merged
merged 3 commits into from
Apr 14, 2022
Merged

Conversation

aheber
Copy link
Contributor

@aheber aheber commented Mar 30, 2022

What does this PR do?

Enables splitting files at the 10M character line and not just the 10K row line.

Also added a fix and test so we don't create empty batches if we have exactly 10K rows of input.

What issues does this PR fix or reference?

forcedotcom/cli#1460

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @aheber-doterra to sign the Salesforce.com Contributor License Agreement.

@aheber
Copy link
Contributor Author

aheber commented Mar 30, 2022

Will want to discuss decisions made here. I'm adding a new direct dependency (already an indirect dep of csv-parser so no actual additional packages installed).

This change has performance impact, I think it increased the existing splitting text by 2-3x (~200ms to ~600ms with lots of variation) in my tests on my machine. I don't suspect this is slow enough to tank the change but wanted to call it out.

While I've done research to understand how jsForce was generating the CSVs (same library as I've added) this doesn't guarantee that it will also produce exactly the same CSV as what I'm measuring so the uploaded size could be different, maybe think through any gaps and try and cover them?

@mshanemc mshanemc requested a review from shetzel March 30, 2022 21:24
@uip-robot-zz
Copy link

This issue has been linked to a new work item: W-10923291

Copy link
Contributor

@shetzel shetzel left a comment

Choose a reason for hiding this comment

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

Looks good - thanks! Once we get approval to use csv-stringify and manual testing goes well we can merge it.

@@ -89,6 +89,7 @@
"@salesforce/ts-types": "^1.5.20",
"chalk": "^4.1.0",
"csv-parse": "^4.16.3",
"csv-stringify": "^6.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

jsyk, we need to get approval for all 3rd party libs. This one looks like it shouldn't be a problem.

@shetzel
Copy link
Contributor

shetzel commented Apr 4, 2022

I tested with a small and large csv and everything bulk uploaded just fine. I didn't see any significant degradation in performance. Once the library is approved (should be sometime this week) we can merge this.

@aheber
Copy link
Contributor Author

aheber commented Apr 4, 2022

Great news, appreciate the update.

@shetzel shetzel merged commit 26abbd7 into salesforcecli:main Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants