Limit join result length to 10240 #1856
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kchadha
reviewed
Dec 17, 2018
Contributor
kchadha
left a comment
kchadha
left a comment
There was a problem hiding this comment.
LG, but I'm wondering about performance...
How about:
const string1 = Cast.toString(args.STRING1);
const string1Length = string1.length;
if (string1Length >= 10240) return string1;
return string1 + Cast.toString(args.STRING2).slice(0, 10240 - string1Length);
It's more verbose but avoids unnecessary computation..
@mzgoddard, since you're moving back to performance related things, I'm wondering if you have thoughts on this?
Contributor
|
@kchadha had already reviewed - I think this just didn't get assigned back to @paulkaplan. I took a very quick look at performance and doing the split is 65% slower than not, though I assumed the strings were always the following.
I don't know if it would actually slow things down, but @kchadha's suggestions seems reasonable. Checking for length of those two string first did not seem to slow anything down. |
Contributor
|
I think it's too late to add this change - what do you think? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves
What Github issue does this resolve (please include link)?
Resolves #922
Proposed Changes
Describe what this Pull Request does
Limit the result of join the same way scratch-flash did.
Reason for Changes
Explain why these changes should be made
#922
Test Coverage
Please show how you have added tests to cover your changes
Added a unit test for it.