You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "NarekDW (via GitHub)" <gi...@apache.org> on 2023/01/24 15:12:19 UTC

[GitHub] [spark] NarekDW opened a new pull request, #39719: [SPARK-42169] Implement code generation for to_csv function (StructsToCsv)

NarekDW opened a new pull request, #39719:
URL: https://github.com/apache/spark/pull/39719

   ### What changes were proposed in this pull request?
   This PR enhances `StructsToCsv` class with `doGenCode` function instead of extending it from `CodegenFallback` trait (performance improvement).
   
   ### Why are the changes needed?
   It will improve performance.
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   an additional test case were added to `org.apache.spark.sql.CsvFunctionsSuite` class.
   


-- 
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

[GitHub] [spark] NarekDW commented on a diff in pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #39719:
URL: https://github.com/apache/spark/pull/39719#discussion_r1174382698


##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -577,4 +578,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {

Review Comment:
   Sure, `checkEvaluation` will execute this test in both modes without codegen and with codegen modes. In case of codegen mode, if the generated java code (method) will be larger than 64 kb in size it won't be able to compile it, and the test will fail, as Java has a 64kb limit on the size of methods.
   It doesn't duplicate `to_csv - struct` test case, because the purpose of this test - is to generate big StructType(with 5000 literals in current case) and test it on `StructsToCsv` expression to be sure that generated java code doesn't exceed the limit of 64 kb in size.



-- 
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

[GitHub] [spark] jaceklaskowski commented on a diff in pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "jaceklaskowski (via GitHub)" <gi...@apache.org>.
jaceklaskowski commented on code in PR #39719:
URL: https://github.com/apache/spark/pull/39719#discussion_r1172921608


##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -574,4 +575,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {
+    val range = Range.inclusive(1, 5000)
+    val struct = CreateStruct.create(range.map(Literal(_)))

Review Comment:
   nit: `Literal.apply`



-- 
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

[GitHub] [spark] MaxGekk closed pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)
URL: https://github.com/apache/spark/pull/39719


-- 
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

[GitHub] [spark] NarekDW commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1413818754

   @MaxGekk 
   This is from master branch:
   
   <img width="981" alt="image" src="https://user-images.githubusercontent.com/24260474/216346895-1b25683c-186a-47ce-be50-e912255f061e.png">
   
   This is from current branch:
   
   <img width="984" alt="image" src="https://user-images.githubusercontent.com/24260474/216347344-14bbb4ee-0915-4228-b2a6-f032ee4bd909.png">
   
   On my local machine - `to_csv` related operations in average are about ~20 % faster with this change in `CSVBenchmark`.


-- 
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

[GitHub] [spark] NarekDW commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1517635600

   @jaceklaskowski thank you for the review. @MaxGekk just a reminder.


-- 
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

[GitHub] [spark] NarekDW commented on a diff in pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #39719:
URL: https://github.com/apache/spark/pull/39719#discussion_r1174378524


##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -577,4 +578,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {
+    val range = Range.inclusive(1, 5000)
+    val struct = CreateStruct.create(range.map(Literal.apply))
+    val expected = range.mkString(",")
+    checkEvaluation(StructsToCsv(Map.empty, struct), expected)

Review Comment:
   Yep, done.



-- 
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

[GitHub] [spark] MaxGekk commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1617503523

   +1, LGTM. Merging to master.
   Thank you, @NarekDW and @HyukjinKwon @jaceklaskowski for 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

[GitHub] [spark] NarekDW commented on a diff in pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #39719:
URL: https://github.com/apache/spark/pull/39719#discussion_r1173620399


##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -574,4 +575,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {
+    val range = Range.inclusive(1, 5000)
+    val struct = CreateStruct.create(range.map(Literal(_)))

Review Comment:
   done



-- 
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

[GitHub] [spark] NarekDW commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1616435203

   @MaxGekk sorry for delay, benchmark results are updated.


-- 
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

[GitHub] [spark] MaxGekk commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1417051227

   @NarekDW Could you regenerate benchmark results using GitHub actions, see
   https://spark.apache.org/developer-tools.html (Running benchmarks in your forked repository)
   and update:
   - CSVBenchmark-results.txt
   - CSVBenchmark-jdk11-results.txt
   - CSVBenchmark-jdk17-results.txt
   
   in your 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.

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

[GitHub] [spark] MaxGekk commented on a diff in pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #39719:
URL: https://github.com/apache/spark/pull/39719#discussion_r1174354578


##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -577,4 +578,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {

Review Comment:
   Could you clarify this test title, please. I don't see anything related to 64KB in the test.
   
   Doesn't the test just duplicates existing one: `to_csv - struct`?



##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -577,4 +578,11 @@ class CsvFunctionsSuite extends QueryTest with SharedSparkSession {
       $"csv", schema_of_csv("1,2\n2"), Map.empty[String, String].asJava))
     checkAnswer(actual, Row(Row(1, "2\n2")))
   }
+
+  test("StructsToCsv should not generate codes beyond 64KB") {
+    val range = Range.inclusive(1, 5000)
+    val struct = CreateStruct.create(range.map(Literal.apply))
+    val expected = range.mkString(",")
+    checkEvaluation(StructsToCsv(Map.empty, struct), expected)

Review Comment:
   It is better to place the test to `CsvExpressionsSuite` because `CsvFunctionsSuite` aims to check functions like `to_csv`/`from_csv`.



-- 
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

[GitHub] [spark] MaxGekk commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1611091872

   @NarekDW Could you rebase this PR on the recent master, 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.

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

[GitHub] [spark] NarekDW commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1611793513

   @MaxGekk sure, I've rebased, but there were conflicts in benchmarks. It will take some time to regenerate them.
   


-- 
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

[GitHub] [spark] NarekDW commented on pull request #39719: [SPARK-42169] [SQL] Implement code generation for to_csv function (StructsToCsv)

Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on PR #39719:
URL: https://github.com/apache/spark/pull/39719#issuecomment-1427127282

   > @NarekDW Could you regenerate benchmark results using GitHub actions, see https://spark.apache.org/developer-tools.html (Running benchmarks in your forked repository) and update:
   > 
   > * CSVBenchmark-results.txt
   > * CSVBenchmark-jdk11-results.txt
   > * CSVBenchmark-jdk17-results.txt
   > 
   > in your PR.
   
   @MaxGekk sorry for the late response. I've added benchmark results from GitHub actions.
   I've added 2 commits: in pre-last commit I've updated benchmarks results from **master** branch and in the last commit I've added  benchmarks results from **current** branch.
   
   Execution links for master branch:
   [Java 8](https://github.com/NarekDW/spark/actions/runs/4151578967)
   [Java 11](https://github.com/NarekDW/spark/actions/runs/4151828105)
   [Java 17](https://github.com/NarekDW/spark/actions/runs/4152154541)
   
   Execution links for current branch:
   [Java 8](https://github.com/NarekDW/spark/actions/runs/4152491256)
   [Java 11](https://github.com/NarekDW/spark/actions/runs/4152723531)
   [Java 17](https://github.com/NarekDW/spark/actions/runs/4155468338)
   
   P.S. **Scala 2.12** was used for all executions.


-- 
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