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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
