You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2025/07/15 03:13:47 UTC
[PR] [MINOR][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
yaooqinn opened a new pull request, #51488:
URL: https://github.com/apache/spark/pull/51488
### What changes were proposed in this pull request?
This PR fixes ThriftServerQueryTestSuite result comparison. While reading the Golden Files back, we remove the sort operation as these archived results are already sorted.
### Why are the changes needed?
For a multiline result of a single row, it might get malformed, for example
```
[info] Expected "[ <birth>2018</birth>
[info] <name>[45 61 73 6F 6E]</name>
[info] <org>[4B 69 6E 64 65 72 67 61 72 74 65 6E 20 43 6F 70]</org>
[info] </ROW>
[info] <]ROW>", but got "[<ROW>
[info] <name>[45 61 73 6F 6E]</name>
[info] <birth>2018</birth>
[info] <org>[4B 69 6E 64 65 72 67 61 72 74 65 6E 20 43 6F 70]</org>
[info] </]ROW>"
```
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Tested with https://github.com/apache/spark/pull/51470 locally
### Was this patch authored or co-authored using generative AI tooling?
No
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51488:
URL: https://github.com/apache/spark/pull/51488#issuecomment-3072628353
Thank you @dongjoon-hyun, SPARK-52799 is filed
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2206944231
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
+ splits.sliding(step, step).map(_.mkString("\n")).toSeq.sorted.mkString("\n")
Review Comment:
Thank you @peter-toth
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51488:
URL: https://github.com/apache/spark/pull/51488#issuecomment-3073014030
Merged to master and 4.0, thank you @dongjoon-hyun and @peter-toth for the review
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2282194182
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
Review Comment:
but `rowCounts(i)` is based on the calculated result, while `splits` is from the original output in the golden file.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2280390362
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
Review Comment:
Then splits is empty and not entering this if- branch
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2206928792
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
+ splits.sliding(step, step).map(_.mkString("\n")).toSeq.sorted.mkString("\n")
Review Comment:
`.sliding(step, step)` -> `.grouped(step)`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2206792549
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
Review Comment:
Assuming `\n`s are added equivalently to each row according to the schema
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51488:
URL: https://github.com/apache/spark/pull/51488#discussion_r2280185461
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala:
##########
@@ -162,11 +164,19 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite with SharedThriftServ
val sql = segments(i * 3 + 1).trim
val schema = segments(i * 3 + 2).trim
val originalOut = segments(i * 3 + 3)
- val output = if (schema != emptySchema && isNeedSort(sql)) {
- originalOut.split("\n").sorted.mkString("\n")
- } else {
- originalOut
- }
+ val output =
+ if (schema != emptySchema && isNeedSort(sql)) {
+ val splits = originalOut.split("\n")
+ if (splits.length > rowCounts(i)) {
+ // the result is multiline
+ val step = splits.length / rowCounts(i)
Review Comment:
will `rowCounts(i)` be 0?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #51488: [SPARK-52799][TESTS] Fix ThriftServerQueryTestSuite result comparison
URL: https://github.com/apache/spark/pull/51488
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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