You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "calilisantos (via GitHub)" <gi...@apache.org> on 2025/03/06 19:51:30 UTC

[PR] [SPARK-51402][SQL][TESTS] Test TimeType in UDF [spark]

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

   ## **What changes were proposed in this pull request?**
   Write tests for TimeType in UDF as input parameters and results.
   
   ## **Why are the changes needed?**
   It follows https://issues.apache.org/jira/browse/SPARK-51402 to improve test coverage.
   
   ## **Does this PR introduce any user-facing change?**
   No
   
   ## **How was this patch tested?**
   Unit test


-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,35 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Mocks
+    val mockTimeStr = "00:00:00.000000"
+    val input = Seq(java.time.LocalTime.parse(mockTimeStr)).toDF("currentTime")
+    // Regular case
+    val plusHour = udf((l: java.time.LocalTime) => l.plusHours(1))
+    val result = input.select(plusHour($"currentTime").cast(TimeType()).as("newTime"))

Review Comment:
   perfect, i adjusted this case, now checking the schema resulted of the UDF



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
dev/create-release/release-build.sh:
##########
@@ -137,6 +137,12 @@ if [[ "$1" == "finalize" ]]; then
     --repository-url https://upload.pypi.org/legacy/ \
     "pyspark_connect-$PYSPARK_VERSION.tar.gz" \
     "pyspark_connect-$PYSPARK_VERSION.tar.gz.asc"
+  svn update "pyspark-client-$RELEASE_VERSION.tar.gz"
+  svn update "pyspark-client-$RELEASE_VERSION.tar.gz.asc"
+  TWINE_USERNAME=spark-upload TWINE_PASSWORD="$PYPI_PASSWORD" twine upload \
+    --repository-url https://upload.pypi.org/legacy/ \
+    "pyspark-client-$RELEASE_VERSION.tar.gz" \
+    "pyspark-client-$RELEASE_VERSION.tar.gz.asc"

Review Comment:
   Please, revert unrelated changes.



##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,35 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Mocks
+    val mockTimeStr = "00:00:00.000000"
+    val input = Seq(java.time.LocalTime.parse(mockTimeStr)).toDF("currentTime")
+    // Regular case
+    val plusHour = udf((l: java.time.LocalTime) => l.plusHours(1))
+    val result = input.select(plusHour($"currentTime").cast(TimeType()).as("newTime"))

Review Comment:
   I guess the case is not needed since the type is `TimeType()` already.



##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   ok, let's leave it but, please, avoid unrelated changes in the future:
   - there are many places in code base where you could fix indentations. It is better to open a separate PR, and focus only on this.
   - the changes can cause merge conflicts in down stream branches



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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

   @calilisantos Congratulations with your first contribution to Apache Spark!


-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,34 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Regular case
+    val input = Seq(java.time.LocalDateTime.parse("2021-01-01T00:00:00.000000")).toDF("dateTime")
+    val plusHour = udf((l: java.time.LocalDateTime) => l.plusHours(1))
+    val result = input.select(plusHour($"dateTime").cast(TimeType).as("newTime"))

Review Comment:
   Ok!



##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   this is to follow the style guide indentation. Make sense?



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #50194: [SPARK-51402][SQL][TESTS] Test TimeType in UDF
URL: https://github.com/apache/spark/pull/50194


-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   ok, i got it



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,35 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Mocks
+    val mockTimeStr = "00:00:00.000000"
+    val input = Seq(java.time.LocalTime.parse(mockTimeStr)).toDF("currentTime")

Review Comment:
   Could you import java.time.LocalTime above:
   ```scala
   import java.time.{Instant, LocalDate, LocalTime}
   ```
   This should improve test readability.



##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,35 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Mocks
+    val mockTimeStr = "00:00:00.000000"
+    val input = Seq(java.time.LocalTime.parse(mockTimeStr)).toDF("currentTime")
+    // Regular case
+    val plusHour = udf((l: java.time.LocalTime) => l.plusHours(1))
+    val result = input.select(plusHour($"currentTime").as("newTime"))
+    checkAnswer(result, Row(java.time.LocalTime.parse("01:00:00.000000")) :: Nil)
+    assert(result.schema === new StructType().add("newTime", TimeType()))
+    // UDF produces `null`
+    val nullFunc = udf((_: java.time.LocalTime) => null.asInstanceOf[java.time.LocalTime])
+    val nullResult = input.select(nullFunc($"currentTime").as("nullTime"))
+    checkAnswer(nullResult, Row(null) :: Nil)
+    assert(nullResult.schema === new StructType().add("nullTime", TimeType()))
+    // Input parameter of UDF is null
+    val nullInput = Seq(null.asInstanceOf[java.time.LocalTime]).toDF("nullTime")
+    val constDuration = udf((_: java.time.LocalTime) =>

Review Comment:
   ```suggestion
       val constTime = udf((_: java.time.LocalTime) =>
   ```



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   ok, i got it. Thank you!



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   Are the changes necessary?



##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -1197,6 +1197,34 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       Row(Row(null)))
   }
 
+  test("SPARK-51402: Test TimeType in UDF") {
+    // Regular case
+    val input = Seq(java.time.LocalDateTime.parse("2021-01-01T00:00:00.000000")).toDF("dateTime")
+    val plusHour = udf((l: java.time.LocalDateTime) => l.plusHours(1))
+    val result = input.select(plusHour($"dateTime").cast(TimeType).as("newTime"))

Review Comment:
   Cast a timestamp to time has not been supported yet, please, use `LocalTime` directly.



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   ok, i got it. Thank you!



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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

   > @calilisantos Congratulations with your first contribution to Apache Spark!
   
   Thanks @MaxGekk, see you next time!


-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
dev/create-release/release-build.sh:
##########
@@ -137,6 +137,12 @@ if [[ "$1" == "finalize" ]]; then
     --repository-url https://upload.pypi.org/legacy/ \
     "pyspark_connect-$PYSPARK_VERSION.tar.gz" \
     "pyspark_connect-$PYSPARK_VERSION.tar.gz.asc"
+  svn update "pyspark-client-$RELEASE_VERSION.tar.gz"
+  svn update "pyspark-client-$RELEASE_VERSION.tar.gz.asc"
+  TWINE_USERNAME=spark-upload TWINE_PASSWORD="$PYPI_PASSWORD" twine upload \
+    --repository-url https://upload.pypi.org/legacy/ \
+    "pyspark-client-$RELEASE_VERSION.tar.gz" \
+    "pyspark-client-$RELEASE_VERSION.tar.gz.asc"

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

Re: [PR] [SPARK-51402][SQL][TESTS] Test TimeType in UDF [spark]

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


##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -862,7 +862,7 @@ class UDFSuite extends QueryTest with SharedSparkSession {
       .select(myUdf1(Column("col"))),
     Row(ArrayBuffer(100)))
 
-     val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
+    val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>

Review Comment:
   ok, i got it. Thank you!



-- 
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-51402][SQL][TESTS] Test TimeType in UDF [spark]

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

   +1, LGTM. Merging to master.
   Thank you, @calilisantos.


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