[SPARK-25829][SQL][FOLLOWUP] Refactor MapConcat in order to check properly the limit size #23217
Conversation
…perly the limit size
|
cc @cloud-fan |
|
|
||
| val index = keyToIndex.getOrDefault(key, -1) | ||
| if (index == -1) { | ||
| if (withSizeCheck && size >= ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) { |
There was a problem hiding this comment.
I think we should always check the size. Such a big map is very likely to cause problems.
There was a problem hiding this comment.
this flag is just for perf reasons, we can skip the check in some conditions and I didn't want to introduce perf overhead if not needed. If we remove the flag we would do the comparison for each item, also when it is not needed.
There was a problem hiding this comment.
hmmm, I'd like to avoid premature optimization. Actually how much perf this can save? This code block is already doing some heavy work.
There was a problem hiding this comment.
ok, let me remove it then, thanks.
| val index = keyToIndex.getOrDefault(key, -1) | ||
| if (index == -1) { | ||
| if (withSizeCheck && size >= ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) { | ||
| throw new RuntimeException(s"Unsuccessful attempt to concat maps with $size elements " + |
|
thanks for the cleanup! |
|
Test build #99664 has finished for PR 23217 at commit
|
|
Test build #99670 has finished for PR 23217 at commit
|
|
retest this please |
|
Test build #99668 has finished for PR 23217 at commit
|
|
Test build #99680 has finished for PR 23217 at commit
|
|
thanks, merging to master! |
…perly the limit size ## What changes were proposed in this pull request? The PR starts from the [comment](apache#23124 (comment)) in the main one and it aims at: - simplifying the code for `MapConcat`; - be more precise in checking the limit size. ## How was this patch tested? existing tests Closes apache#23217 from mgaido91/SPARK-25829_followup. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
3 participants
What changes were proposed in this pull request?
The PR starts from the comment in the main one and it aims at:
MapConcat;How was this patch tested?
existing tests