You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-db (via GitHub)" <gi...@apache.org> on 2025/07/19 21:54:56 UTC
[PR] [WIP][SPARK-52883][SPARK-52884] Implement the to_time and try_to_time functions in Scala [spark]
uros-db opened a new pull request, #51575:
URL: https://github.com/apache/spark/pull/51575
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Implement the `to_time` and `try_to_time` functions in Scala API.
### Why are the changes needed?
Expand API support for the `ToTime` and `TryToTime` expressions.
### Does this PR introduce _any_ user-facing change?
Yes, the new functions are now available in Scala API.
### How was this patch tested?
Added appropriate Scala functions tests.
### 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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2224755528
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
+ // Input data for the function.
+ val schema = StructType(Seq(
+ StructField("str", StringType, nullable = false)
+ ))
+ val data = Seq(
+ Row("00:00:00"),
+ Row("01:02:03.4"),
+ Row("23:59:59.999999")
+ )
+ val df = spark.createDataFrame(spark.sparkContext.parallelize(data), schema)
+
+ // Test the function using both `selectExpr` and `select`.
+ val result1 = df.selectExpr(
+ "to_time(str)"
+ )
+ val result2 = df.select(
+ to_time(col("str"))
+ )
+ // Check that both methods produce the same result.
+ checkAnswer(result1, result2)
+
+ // Expected output of the function.
+ val expected = Seq(
+ "00:00:00",
+ "01:02:03.4",
+ "23:59:59.999999"
+ ).toDF("timeString").select(col("timeString").cast("time"))
+ // Check that the results match the expected output.
+ checkAnswer(result1, expected)
+ checkAnswer(result2, expected)
+ }
+
+ test("SPARK-52883: to_time function with format") {
Review Comment:
I believe we should cover the elements of function behaviour highlighted in its description:
"
A time, or **raises an error** if the input is malformed.
"
Apparently, the test should check:
1. At least one regular case of the dataframe function `to_time`
2. An error
3. Optionally some corner cases
4. Optionally selectExpr
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51575:
URL: https://github.com/apache/spark/pull/51575#issuecomment-3094332520
@MaxGekk Please 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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #51575:
URL: https://github.com/apache/spark/pull/51575#issuecomment-3108856523
The test failure is not related to the changes I believe:
```
[info] OracleIntegrationSuite:
[info] org.apache.spark.sql.jdbc.OracleIntegrationSuite *** ABORTED *** (10 minutes, 17 seconds)
[info] The code passed to eventually never returned normally. Attempted 599 times over 10.014264630083334 minutes. Last failure message: ORA-12541: Cannot connect. No listener at host 10.1.0.70 port 33247. (CONNECTION_ID=Yl6YcC1+QPeTgz4oTMOnCg==)
```
+1, LGTM. Merging to master.
Thank you, @uros-db.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2221910400
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
Review Comment:
```suggestion
* A string to be parsed to time
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
Review Comment:
```suggestion
* A string to be parsed to time
```
##########
python/pyspark/sql/tests/test_functions.py:
##########
@@ -81,7 +81,12 @@ def test_function_parity(self):
missing_in_py = jvm_fn_set.difference(py_fn_set)
# Functions that we expect to be missing in python until they are added to pyspark
- expected_missing_in_py = set()
+ expected_missing_in_py = set(
+ [
+ "to_time", # TODO(SPARK-52890): @uros-db is addressing this separately.
+ "try_to_time", # TODO(SPARK-52891): @uros-db is addressing this separately.
Review Comment:
```suggestion
# TODO(SPARK-52890): Implement the to_time function in Python
"to_time",
# TODO(SPARK-52891): Implement the try_to_time function in Python
"try_to_time",
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @param format
+ * time format pattern to follow
Review Comment:
Could you refer to doc page with patterns, please.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
Review Comment:
Could you move `If the input is malformed, raises an error.` under the `@return`. See to_timestamp and to_date
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223854016
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
For scala functions, there really isn't a proposed uniform format for `@param`. Instead, the widely used format seems to be: start with lowercase, no period at the end.
For some functions, it's: start with uppercase, no period at the end.
And for a minimal number of functions, there's a period at the end.
In conclusion, I don't think there's really strict formatting here, but let's stick with a single approach across TIME-related functions. I'm fine with either way, so let's stick with you current proposal: start with uppercase, period at the end.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or raises an error if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
Same as https://github.com/apache/spark/pull/51575/files/a38b76dce876b8464ed30b582791ed966e84b5b6#r2223270557.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or raises an error if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @param format
+ * Time format pattern to follow
Review Comment:
Same as https://github.com/apache/spark/pull/51575/files/a38b76dce876b8464ed30b582791ed966e84b5b6#r2223270557.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2225256911
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
+ // Input data for the function.
+ val schema = StructType(Seq(
+ StructField("str", StringType, nullable = false)
+ ))
+ val data = Seq(
+ Row("00:00:00"),
+ Row("01:02:03.4"),
+ Row("23:59:59.999999")
+ )
+ val df = spark.createDataFrame(spark.sparkContext.parallelize(data), schema)
+
+ // Test the function using both `selectExpr` and `select`.
+ val result1 = df.selectExpr(
+ "to_time(str)"
+ )
+ val result2 = df.select(
+ to_time(col("str"))
+ )
+ // Check that both methods produce the same result.
+ checkAnswer(result1, result2)
+
+ // Expected output of the function.
+ val expected = Seq(
+ "00:00:00",
+ "01:02:03.4",
+ "23:59:59.999999"
+ ).toDF("timeString").select(col("timeString").cast("time"))
+ // Check that the results match the expected output.
+ checkAnswer(result1, expected)
+ checkAnswer(result2, expected)
+ }
+
+ test("SPARK-52883: to_time function with format") {
Review Comment:
Makes sense, I updated the tests.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2222883721
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
Review Comment:
Added @returns.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2222801960
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
Review Comment:
Fixed.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
Review Comment:
Fixed.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2217748094
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuite.scala:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types._
+
+class TimeFunctionsSuite extends QueryTest with SharedSparkSession {
+ import testImplicits._
+
+ override def sparkConf: SparkConf = super.sparkConf.set(SQLConf.ANSI_ENABLED.key, "false")
+
+ test("SPARK-52883: to_time function without format") {
Review Comment:
Note for reviewers:
We already have thorough testing of the underlying catalyst expressions in `TimeExpressionsSuite.scala`. These tests are intended to fully cover the expected behavior - incl. error handling, null handling, etc.
The purpose in this PR is only to ensure that Scala API functions are properly documented and registered, as well as to provide minimal testing to ensure connectivity with the underlying catalyst expressions / SQL functions. With this in mind, I would propose to do only minimal testing as part of this PR, in order to avoid exploding the set of test-case scenarios (which are already covered in `TimeExpressionsSuite.scala`). Edge case testing should not be the target / goal of this 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
Re: [PR] [SPARK-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2222883721
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
Review Comment:
Added @return.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223265937
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or null if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def try_to_time(str: Column): Column = {
+ Column.fn("try_to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @param format
+ * Time format pattern to follow
Review Comment:
nit:
```suggestion
* A time format pattern to follow.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
+ // Input data for the function.
+ val schema = StructType(Seq(
+ StructField("str", StringType, nullable = false)
+ ))
+ val data = Seq(
+ Row("00:00:00"),
+ Row("01:02:03.4"),
+ Row("23:59:59.999999")
+ )
+ val df = spark.createDataFrame(spark.sparkContext.parallelize(data), schema)
+
+ // Test the function using both `selectExpr` and `select`.
+ val result1 = df.selectExpr(
+ "to_time(str)"
+ )
+ val result2 = df.select(
+ to_time(col("str"))
+ )
+ // Check that both methods produce the same result.
+ checkAnswer(result1, result2)
+
+ // Expected output of the function.
+ val expected = Seq(
+ "00:00:00",
+ "01:02:03.4",
+ "23:59:59.999999"
+ ).toDF("timeString").select(col("timeString").cast("time"))
+ // Check that the results match the expected output.
+ checkAnswer(result1, expected)
+ checkAnswer(result2, expected)
+ }
+
+ test("SPARK-52883: to_time function with format") {
Review Comment:
Could you add a negative test w/ an error, please.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or raises an error if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @param format
+ * Time format pattern to follow
Review Comment:
```suggestion
* A time format pattern to follow.
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
nit:
```suggestion
* A string to be parsed to time.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
Review Comment:
Please, add a negative test where the function raises an error.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
nit:
```suggestion
* A string to be parsed to time.
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or raises an error if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
nit:
```suggestion
* A string to be parsed to time.
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or null if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def try_to_time(str: Column): Column = {
+ Column.fn("try_to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
```suggestion
* A string to be parsed to 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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223811679
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
Review Comment:
Let's discuss in https://github.com/apache/spark/pull/51575#discussion_r2223261605.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223854597
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
Same as https://github.com/apache/spark/pull/51575/files/a38b76dce876b8464ed30b582791ed966e84b5b6#r2223270557.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or null if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def try_to_time(str: Column): Column = {
+ Column.fn("try_to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
Same as https://github.com/apache/spark/pull/51575/files/a38b76dce876b8464ed30b582791ed966e84b5b6#r2223270557.
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5731,6 +5764,39 @@ object functions {
*/
def to_timestamp(s: Column, fmt: String): Column = Column.fn("to_timestamp", s, lit(fmt))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @return
+ * A time, or null if the input is malformed.
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def try_to_time(str: Column): Column = {
+ Column.fn("try_to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value.
+ *
+ * See <a href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html"> Datetime
+ * Patterns</a> for valid time format patterns.
+ *
+ * @param str
+ * A string to be parsed to time
+ * @param format
+ * Time format pattern to follow
Review Comment:
Same as https://github.com/apache/spark/pull/51575/files/a38b76dce876b8464ed30b582791ed966e84b5b6#r2223270557.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223856883
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5699,6 +5699,39 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value.
+ *
+ * @param str
+ * A string to be parsed to time
Review Comment:
Updated all descriptions accordingly.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2222799281
##########
python/pyspark/sql/tests/test_functions.py:
##########
@@ -81,7 +81,12 @@ def test_function_parity(self):
missing_in_py = jvm_fn_set.difference(py_fn_set)
# Functions that we expect to be missing in python until they are added to pyspark
- expected_missing_in_py = set()
+ expected_missing_in_py = set(
+ [
+ "to_time", # TODO(SPARK-52890): @uros-db is addressing this separately.
+ "try_to_time", # TODO(SPARK-52891): @uros-db is addressing this separately.
Review Comment:
Fixed.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2223811295
##########
sql/core/src/test/scala/org/apache/spark/sql/TimeFunctionsSuiteBase.scala:
##########
@@ -161,6 +161,150 @@ abstract class TimeFunctionsSuiteBase extends QueryTest with SharedSparkSession
checkAnswer(result1, expected)
checkAnswer(result2, expected)
}
+
+ test("SPARK-52883: to_time function without format") {
+ // Input data for the function.
+ val schema = StructType(Seq(
+ StructField("str", StringType, nullable = false)
+ ))
+ val data = Seq(
+ Row("00:00:00"),
+ Row("01:02:03.4"),
+ Row("23:59:59.999999")
+ )
+ val df = spark.createDataFrame(spark.sparkContext.parallelize(data), schema)
+
+ // Test the function using both `selectExpr` and `select`.
+ val result1 = df.selectExpr(
+ "to_time(str)"
+ )
+ val result2 = df.select(
+ to_time(col("str"))
+ )
+ // Check that both methods produce the same result.
+ checkAnswer(result1, result2)
+
+ // Expected output of the function.
+ val expected = Seq(
+ "00:00:00",
+ "01:02:03.4",
+ "23:59:59.999999"
+ ).toDF("timeString").select(col("timeString").cast("time"))
+ // Check that the results match the expected output.
+ checkAnswer(result1, expected)
+ checkAnswer(result2, expected)
+ }
+
+ test("SPARK-52883: to_time function with format") {
Review Comment:
I kind of described this in: https://github.com/apache/spark/pull/51575#discussion_r2217748094.
TLDR: I don't think that we need to go super deep with scala function tests here. We already have extensive testing, so here I would focus on the bare minimum. Otherwise, we explode the testing space quite a bit, if we want to do this for every expression/function.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51575:
URL: https://github.com/apache/spark/pull/51575#discussion_r2222821789
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -5683,6 +5683,32 @@ object functions {
def unix_timestamp(s: Column, p: String): Column =
Column.fn("unix_timestamp", s, lit(p))
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @group datetime_funcs
+ * @since 4.1.0
+ */
+ def to_time(str: Column): Column = {
+ Column.fn("to_time", str)
+ }
+
+ /**
+ * Parses a string value to a time value. If the input is malformed, raises an error.
+ *
+ * @param str
+ * string to be parsed to time
+ * @param format
+ * time format pattern to follow
Review Comment:
Added.
--
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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51575:
URL: https://github.com/apache/spark/pull/51575#issuecomment-3108806334
Docker OracleIntegrationSuite failure in CI looks unrelated. @MaxGekk Please re-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-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #51575: [SPARK-52883][SPARK-52884][SQL] Implement the to_time and try_to_time functions in Scala
URL: https://github.com/apache/spark/pull/51575
--
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