You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/06/08 08:20:42 UTC
[GitHub] [spark] dilipbiswal opened a new pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
dilipbiswal opened a new pull request #28750:
URL: https://github.com/apache/spark/pull/28750
### What changes were proposed in this pull request?
A minor fix to fix the apend method of StringConcat to only increment the length if underline string buffer actually changes in size.
Thanks to Jeffrey Stokes for reporting the issue and explaining the underlying problem in detail in the JIRA.
### Why are the changes needed?
This fixes StringIndexOutOfBoundsException on an overflow.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Added a test in StringsUtilSuite.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642325127
**[Test build #123797 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123797/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641773954
**[Test build #123730 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123730/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642388631
**[Test build #123817 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123817/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436668197
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
Nitpicking, but:
Why not only update length inside the `if (!atLimit)` block as originally shown?
And only add `stringToAppend.length()` there, if the point of `Math.min()` is to avoid overflow if adding a massive string where only part was added.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
Oh I see. If overflow is an issue, then make it a long? Capping it at this constant would then seem to potentially underreport the actual appended total.
Maybe that creates other problems, not sure.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
I'm not sure you can overflow a long - if 2^31-1 strings (max length of the array tracking them) are added, of length 2^31-1, you still don't overflow.
It's probably not worth debating so this is fine but I think a simpler change was also fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640450409
**[Test build #123622 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123622/testReport)** for PR 28750 at commit [`8d723d9`](https://github.com/apache/spark/commit/8d723d9613f2e4cc3546082042923255e57d6fb0).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640447472
cc @maropu
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766412
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/123728/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642499725
**[Test build #123837 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123837/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766107
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640499311
**[Test build #123626 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123626/testReport)** for PR 28750 at commit [`0e71179`](https://github.com/apache/spark/commit/0e71179e72deefbc54b711b75c2b541abaf1d933).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu closed pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu closed pull request #28750:
URL: https://github.com/apache/spark/pull/28750
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641078762
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642389012
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642323317
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640451062
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641938532
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/123730/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641773954
**[Test build #123730 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123730/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436535675
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala
##########
@@ -98,4 +98,12 @@ class StringUtilsSuite extends SparkFunSuite {
assert(checkLimit("1234567"))
assert(checkLimit("1234567890"))
}
+
+ test("StringConcat doesn't overflow on many inputs") {
Review comment:
@kiszk Thanks.. will do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436548756
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@MaxGekk Thanks a lot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r437998610
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -123,7 +123,11 @@ object StringUtils extends Logging {
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
}
- length += sLen
+
+ // Cap the length at ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH. Otherwise, we
+ // will overflow length causing StringIndexOutOfBoundsException in the substring call
+ // above.
Review comment:
@dilipbiswal Sorry and my comment looked ambiguous. I just wanted to leave a note about your comment above;
> I think, the original intent of keeping this length outside the if block (which i missed and @MaxGekk pointed out) is to keep the true length of the input. So when we are producing a SQL plan, we want to tell users how much was that the size of the explain string and how much we truncated based on max length specification. So caller could call append method even after we have gone past the max.
I couldn't tell why the count-up is placed outside `the if (!atLimit) block`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal edited a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal edited a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-643019799
Hi @maropu
Just checked the 2.4 codebase. We don't have this Class in 2.4. Looks like we directed appended to StringBuilder. This class was added as part of [SPARK-26504] (https://issues.apache.org/jira/browse/SPARK-26504) and fix version is 3.0.0
Please let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766399
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640451062
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r438015265
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -123,7 +123,11 @@ object StringUtils extends Logging {
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
}
- length += sLen
+
+ // Cap the length at ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH. Otherwise, we
+ // will overflow length causing StringIndexOutOfBoundsException in the substring call
+ // above.
Review comment:
@maropu Ah .. so you wanted to explain the above comment better in the PR and not in the code ? Right ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436808519
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@srowen I think, the original intent of keeping this length outside the if block (which i missed and @MaxGekk pointed out) is to keep the true length of the input. So when we are producing a SQL plan, we want to tell users how much was that the size of the explain string and how much we truncated based on max length specification. So caller could call append method even after we have gone past the max.
cc @MaxGekk
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@srowen You are right that, we may under report when the length becomes more than MAX_ROUNDED_ARRAY_LENGTH. I did consider changing the message to cover the case to say something like "greater than or equal to N". But thought against it as we are adding code to cover may be a extremely corner case. Please let me know if you have any suggestion here.
About changing to long, even if we did it, we can in theory hit the long limit, right ?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
Thank you @srowen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641937376
**[Test build #123730 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123730/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642454870
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640500027
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641765460
**[Test build #123728 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123728/testReport)** for PR 28750 at commit [`1050df3`](https://github.com/apache/spark/commit/1050df32690bd4a1ad9fd92cf680a63ff41cbf68).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642499725
**[Test build #123837 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123837/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640583011
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766399
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641765460
**[Test build #123728 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123728/testReport)** for PR 28750 at commit [`1050df3`](https://github.com/apache/spark/commit/1050df32690bd4a1ad9fd92cf680a63ff41cbf68).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436539108
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
How about:
```scala
length = Math.min(length.toLong + sLen, ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH).toInt
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-643031823
Ah, okay. We don't need the backport. Thanks for the check, anyway!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766979
@maropu
> Have you checked my last comment? #28750 (comment) The PR itself looks okay.
Sorry i missed that. I have added the comment now.
@viirya
We could make it a long. The only thing is we may still overflow but it will take perhaps long time to hit it. I can repro the StringIndexOutOfBoundsException after changing the length to long. I made a minor tweak to change the append method to fake the input's length to be Int.MaxValue and adjust the test to increase the loop count.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641774562
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436540551
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@MaxGekk Yeah.. Sure. Just wondering if we absolutely need to change the length in case we are not doing an append. The error message you are referring to, is this captured in some test ? I ran catalyst and sql tests and didn't see any failure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r437158187
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -29,6 +29,7 @@ import org.apache.spark.sql.catalyst.expressions.aggregate.{Complete, Partial}
import org.apache.spark.sql.catalyst.optimizer.{ConvertToLocalRelation, NestedColumnAliasingSuite}
import org.apache.spark.sql.catalyst.plans.logical.Project
import org.apache.spark.sql.catalyst.util.StringUtils
+import org.apache.spark.sql.catalyst.util.StringUtils.PlanStringConcat
Review comment:
unnecessary change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640583011
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436545452
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
Review comment:
@kiszk If i incorporate @MaxGekk's suggestion, then i will need to keep 120 where it is :-) as we need it outside the if block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r438066830
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -123,7 +123,11 @@ object StringUtils extends Logging {
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
}
- length += sLen
+
+ // Cap the length at ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH. Otherwise, we
+ // will overflow length causing StringIndexOutOfBoundsException in the substring call
+ // above.
Review comment:
The comment in the code side looks okay and how about this?
```
// Keeps the total length of appended strings. Note that we need to cap the length at
// `ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH`; otherwise, we will overflow
// length causing StringIndexOutOfBoundsException in the substring call above.
length = Math.min(length.toLong + sLen, ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH).toInt
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436540551
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@MaxGekk Yeah.. Sure. Just wondering we absolutely need to change the length in case we are not doing an append. The error message you are referring to, is this captured in some test ? I ran catalyst and sql tests and didn't see any failure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642500357
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642325127
**[Test build #123797 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123797/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642430970
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642389012
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436542652
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
> The error message you are referring to, is this captured in some test ?
Looks like not. Could you add a test for that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642454879
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/123817/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-643005468
@maropu Sure. Thanks a lot for your comments on the pr.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766373
**[Test build #123728 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123728/testReport)** for PR 28750 at commit [`1050df3`](https://github.com/apache/spark/commit/1050df32690bd4a1ad9fd92cf680a63ff41cbf68).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal edited a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal edited a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766979
@maropu
> Have you checked my last comment? #28750 (comment) The PR itself looks okay.
Sorry i missed that. I have added the comment now.
@viirya
> Looks okay although I think making it a long might be also good and simpler.
We could make it a long. The only thing is we may still overflow but it will take perhaps long time to hit it. I can repro the StringIndexOutOfBoundsException after changing the length to long. I made a minor tweak to change the append method to fake the input's length to be Int.MaxValue and adjust the test to increase the loop count.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641938525
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] kiszk commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436536368
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
Review comment:
nit: Can we move line 120 into `{...}` to make the scope sure?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642642988
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642994075
Thanks! Merged to master/branch-3.0.
@dilipbiswal Could you make a backport PR to branch-2.4?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642325426
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642388631
**[Test build #123817 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123817/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640450409
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436544482
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
@MaxGekk Sure.. Will add a test. Can you please point me to the error message please ? The link you have, when i click .. its not taking me to the relevant code ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-643006251
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r437059411
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3503,6 +3504,25 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(sql("select CAST(-32768 as short) DIV CAST (-1 as short)"),
Seq(Row(Short.MinValue.toLong * -1)))
}
+
+
Review comment:
nit: remove the single blank.
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala
##########
@@ -98,4 +98,12 @@ class StringUtilsSuite extends SparkFunSuite {
assert(checkLimit("1234567"))
assert(checkLimit("1234567890"))
}
+
+ test("SPARK-31916: StringConcat doesn't overflow on many inputs") {
+ val concat = new StringConcat(maxLength = 100)
+ 0.to(Integer.MAX_VALUE).foreach { _ =>
Review comment:
Could you reduce # of loops in this test? I think its a bit expensive.
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3503,6 +3504,25 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(sql("select CAST(-32768 as short) DIV CAST (-1 as short)"),
Seq(Row(Short.MinValue.toLong * -1)))
}
+
+
+ test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {
Review comment:
Since `PlanStringConcat's` is in the catalyst package, could you move this tests there?
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3503,6 +3504,25 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(sql("select CAST(-32768 as short) DIV CAST (-1 as short)"),
Seq(Row(Short.MinValue.toLong * -1)))
}
+
+
+ test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {
+ withSQLConf("spark.sql.maxPlanStringLength" -> "0") {
+ val concat = new PlanStringConcat()
+ 0.to(3).foreach { i =>
+ concat.append(s"plan fragment $i")
+ }
+ assert(concat.toString === "Truncated plan of 60 characters")
+ }
+
+ withSQLConf("spark.sql.maxPlanStringLength" -> "60") {
Review comment:
ditto
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3503,6 +3504,25 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(sql("select CAST(-32768 as short) DIV CAST (-1 as short)"),
Seq(Row(Short.MinValue.toLong * -1)))
}
+
+
+ test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {
+ withSQLConf("spark.sql.maxPlanStringLength" -> "0") {
Review comment:
plz use `SQLConf.MAX_PLAN_STRING_LENGTH.key` instead.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
Ah, could you leave some comments about the intent here? I think its a bit hard to tell it at a glance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642430970
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641938525
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-643019799
Hi @maropu
Just checked the 2.4 codebase. We don't have this Class in 2.4. Looks like we directed appended to StringBuilder. This class was added as part of [SPARK-26504](https://issues.apache.org/jira/browse/SPARK-26504)
Please let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642325426
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436547468
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##########
@@ -122,8 +122,8 @@ object StringUtils extends Logging {
val available = maxLength - length
val stringToAppend = if (available >= sLen) s else s.substring(0, available)
strings.append(stringToAppend)
+ length += sLen
}
- length += sLen
Review comment:
In the child class, here https://github.com/apache/spark/blob/d9b30694122f8716d3acb448638ef1e2b96ebc7a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala#L156
https://github.com/apache/spark/blob/d9b30694122f8716d3acb448638ef1e2b96ebc7a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala#L151
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640500027
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642641808
**[Test build #123837 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123837/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642454622
**[Test build #123817 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123817/testReport)** for PR 28750 at commit [`804edd0`](https://github.com/apache/spark/commit/804edd0781ec76a5a625eb0d1538b7aacdb67435).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dilipbiswal commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642498763
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-640582030
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642428805
**[Test build #123797 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123797/testReport)** for PR 28750 at commit [`c7e780b`](https://github.com/apache/spark/commit/c7e780b5081c811c9410df6069ca6016ff7f0b90).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642500357
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641766107
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] kiszk commented on a change in pull request #28750: [SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #28750:
URL: https://github.com/apache/spark/pull/28750#discussion_r436535434
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala
##########
@@ -98,4 +98,12 @@ class StringUtilsSuite extends SparkFunSuite {
assert(checkLimit("1234567"))
assert(checkLimit("1234567890"))
}
+
+ test("StringConcat doesn't overflow on many inputs") {
Review comment:
nit: could you please add the prefix: "SPARK-31916: " ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-641774562
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642642988
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28750: [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28750:
URL: https://github.com/apache/spark/pull/28750#issuecomment-642454870
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org