You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "senthh (via GitHub)" <gi...@apache.org> on 2025/04/06 20:08:34 UTC
[PR] [SPARK-51421][SQL] Get seconds of TIME datatype [spark]
senthh opened a new pull request, #50525:
URL: https://github.com/apache/spark/pull/50525
<!--
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?
This PR adds support for extracting the hour component from TIME (TimeType) values in Spark SQL.
scala> spark.sql("SELECT SECOND('13:59:59.999999');").show()
+----------------------------+
|second('13:59:59.999999)|
+----------------------------+
| 59|
+----------------------------+
scala> spark.sql("SELECT SECOND(TIME'13:59:59.99')").show()
+-------------------------+
|second(TIME'13:59:59.99')|
+-------------------------+
| 59|
+-------------------------+
### Why are the changes needed?
Spark previously supported second() for only TIMESTAMP type values. TIME support was missing, leading to implicit casting attempt to TIMESTAMP, which was incorrect. This PR ensures that hour(TIME'HH:MM:SS.######') behaves correctly without unnecessary type coercion.
### Does this PR introduce _any_ user-facing change?
Yes
- Before this PR, calling second(TIME'HH:MM:SS.######') resulted in a type mismatch error or an implicit cast attempt to TIMESTAMP, which was incorrect.
- With this PR, second(TIME'HH:MM:SS.######') now works correctly for TIME values without implicit casting.
- Users can now extract the second component from TIME values natively.
### How was this patch tested?
By running new tests:
```$ build/sbt "test:testOnly *TimeExpressionsSuite"```
### 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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2030377037
##########
sql/core/src/test/resources/sql-tests/results/hll.sql.out:
##########
@@ -66,9 +70,10 @@ SELECT hll_sketch_estimate(hll_union_agg(sketch, true))
SELECT hll_sketch_agg(col, 20) as sketch
FROM VALUES (1) AS tab(col))
-- !query schema
-struct<hll_sketch_estimate(hll_union_agg(sketch, true)):bigint>
+struct<>
-- !query output
-1
+java.lang.NoClassDefFoundError
+Could not initialize class org.apache.datasketches.memory.internal.BaseWritableMemoryImpl
Review Comment:
@HyukjinKwon I have regenerated hll.sql.out
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2030475356
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -168,4 +168,62 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkConsistencyBetweenInterpretedAndCodegen(
(child: Expression) => MinutesOfTime(child).replacement, TimeType())
}
+
+ test("SecondExpressionBuilder") {
+ // Empty expressions list
+ checkError(
+ exception = intercept[AnalysisException] {
+ SecondExpressionBuilder.build("second", Seq.empty)
+ },
+ condition = "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
+ parameters = Map(
+ "functionName" -> "`second`",
+ "expectedNum" -> "> 0",
+ "actualNum" -> "0",
+ "docroot" -> SPARK_DOC_ROOT)
+ )
+
+ // test TIME-typed child should build SecondsOfTime
+ val timeExpr = Literal(localTime(12, 58, 59), TimeType())
+ val builtExprForTime = SecondExpressionBuilder.build("second", Seq(timeExpr))
+ assert(builtExprForTime.isInstanceOf[SecondsOfTime])
+ assert(builtExprForTime.asInstanceOf[SecondsOfTime].child eq timeExpr)
+
+ // test non TIME-typed child should build second
+ val tsExpr = Literal("2007-09-03 10:45:23")
+ val builtExprForTs = SecondExpressionBuilder.build("second", Seq(tsExpr))
+ assert(builtExprForTs.isInstanceOf[Second])
+ assert(builtExprForTs.asInstanceOf[Second].child eq tsExpr)
+ }
+
+ test("Second with TIME type") {
+ // A few test times in microseconds since midnight:
+ // time in microseconds -> expected second
+ val testTimes = Seq(
Review Comment:
Can you please add these tests to test the output based on precision?
```
val time = "13:10:15.987654321"
Seq(
0 -> 15.toDouble,
1 -> 15.9,
2 -> 15.98,
3 -> 15.987,
4 -> 15.9876,
5 -> 15.98765,
6 -> 15.987654).foreach { case (precision, expected) =>
checkEvaluation(
SecondsOfTime(Literal.create(time, TimeType(precision))),
BigDecimal(expected))
}
```
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2032473882
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
And also test failed with "java.lang.OutOfMemoryError: Java heap space" is not relevant to our changes
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2033210321
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getSecondsOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "second"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+@ExpressionDescription(
+ usage = """
+ _FUNC_(expr) - Returns the second component of the given expression.
+
+ If `expr` is a TIMESTAMP or a string that can be cast to timestamp,
+ it returns the second of that timestamp.
+ If `expr` is a TIME type (since 4.1.0), it returns the second of the time-of-day.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('2018-02-14 12:58:59');
+ 59
+ > SELECT _FUNC_(TIME'13:59:59.999999');
Review Comment:
@MaxGekk Yes good one. I have modified the usage section as below
```
> SELECT _FUNC_(TIME'13:25:59.999999');
```
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2031480480
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
@vinodkc In jira, @MaxGekk has given just IntegerType seconds for an example. So I thought the requirement is just to handle IntegerType. I can modify the implementation so that it should handle both with precision and without.
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2031480480
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
@vinodkc In jira, @MaxGekk has given just IntegerType seconds for example. So I thought the requirement is just to handle IntegerType. I can modify the implementation so that it should handle both with precision and without.
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2031241506
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
Shouldn't it be DecimalType with a precision and scale that matches the precision of TimeType?
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2030497433
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -168,4 +168,62 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkConsistencyBetweenInterpretedAndCodegen(
(child: Expression) => MinutesOfTime(child).replacement, TimeType())
}
+
+ test("SecondExpressionBuilder") {
+ // Empty expressions list
+ checkError(
+ exception = intercept[AnalysisException] {
+ SecondExpressionBuilder.build("second", Seq.empty)
+ },
+ condition = "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
+ parameters = Map(
+ "functionName" -> "`second`",
+ "expectedNum" -> "> 0",
+ "actualNum" -> "0",
+ "docroot" -> SPARK_DOC_ROOT)
+ )
+
+ // test TIME-typed child should build SecondsOfTime
+ val timeExpr = Literal(localTime(12, 58, 59), TimeType())
+ val builtExprForTime = SecondExpressionBuilder.build("second", Seq(timeExpr))
+ assert(builtExprForTime.isInstanceOf[SecondsOfTime])
+ assert(builtExprForTime.asInstanceOf[SecondsOfTime].child eq timeExpr)
+
+ // test non TIME-typed child should build second
+ val tsExpr = Literal("2007-09-03 10:45:23")
+ val builtExprForTs = SecondExpressionBuilder.build("second", Seq(tsExpr))
+ assert(builtExprForTs.isInstanceOf[Second])
+ assert(builtExprForTs.asInstanceOf[Second].child eq tsExpr)
+ }
+
+ test("Second with TIME type") {
+ // A few test times in microseconds since midnight:
+ // time in microseconds -> expected second
+ val testTimes = Seq(
Review Comment:
@vinodkc Sure Vinod. I will create a function for SecondsOfTimeWithFraction and also include tests for the same
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50525:
URL: https://github.com/apache/spark/pull/50525#issuecomment-2789407687
> @MaxGekk Yes Maxx, I have corrected PR description
Thanks.
+1, LGTM. Merging to master.
Thank you, @senthh and @vinodkc @HyukjinKwon 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
Re: [PR] [SPARK-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2031235530
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -168,4 +168,62 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkConsistencyBetweenInterpretedAndCodegen(
(child: Expression) => MinutesOfTime(child).replacement, TimeType())
}
+
+ test("SecondExpressionBuilder") {
+ // Empty expressions list
+ checkError(
+ exception = intercept[AnalysisException] {
+ SecondExpressionBuilder.build("second", Seq.empty)
+ },
+ condition = "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
+ parameters = Map(
+ "functionName" -> "`second`",
+ "expectedNum" -> "> 0",
+ "actualNum" -> "0",
+ "docroot" -> SPARK_DOC_ROOT)
+ )
+
+ // test TIME-typed child should build SecondsOfTime
+ val timeExpr = Literal(localTime(12, 58, 59), TimeType())
+ val builtExprForTime = SecondExpressionBuilder.build("second", Seq(timeExpr))
+ assert(builtExprForTime.isInstanceOf[SecondsOfTime])
+ assert(builtExprForTime.asInstanceOf[SecondsOfTime].child eq timeExpr)
+
+ // test non TIME-typed child should build second
+ val tsExpr = Literal("2007-09-03 10:45:23")
+ val builtExprForTs = SecondExpressionBuilder.build("second", Seq(tsExpr))
+ assert(builtExprForTs.isInstanceOf[Second])
+ assert(builtExprForTs.asInstanceOf[Second].child eq tsExpr)
+ }
+
+ test("Second with TIME type") {
+ // A few test times in microseconds since midnight:
+ // time in microseconds -> expected second
+ val testTimes = Seq(
Review Comment:
@senthh , I updated the above comment; please check
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2032713539
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getSecondsOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
You allow any precision of `TimeType` in `SecondExpressionBuilder`, but here expectsonly `TimeType(6)`. It should work for any valid precision. For now, see:
```sql
spark-sql (default)> select second(cast('12:00:01.123' as time(3)));
[DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE] Cannot resolve "second(CAST(12:00:01.123 AS TIME(3)))" due to data type mismatch: The first parameter requires the "TIME(6)" type, however "CAST(12:00:01.123 AS TIME(3))" has the type "TIME(3)". SQLSTATE: 42K09; line 1 pos 7;
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getSecondsOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "second"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+@ExpressionDescription(
+ usage = """
+ _FUNC_(expr) - Returns the second component of the given expression.
+
+ If `expr` is a TIMESTAMP or a string that can be cast to timestamp,
+ it returns the second of that timestamp.
+ If `expr` is a TIME type (since 4.1.0), it returns the second of the time-of-day.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('2018-02-14 12:58:59');
+ 59
+ > SELECT _FUNC_(TIME'13:59:59.999999');
Review Comment:
It would be better to demonstrate different numbers for seconds and minutes, let's say:
```
> SELECT _FUNC_(TIME'13:10:59.999999');
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
Yep, the type should be `IntergerType`
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2033688063
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getSecondsOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "second"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+@ExpressionDescription(
+ usage = """
+ _FUNC_(expr) - Returns the second component of the given expression.
+
+ If `expr` is a TIMESTAMP or a string that can be cast to timestamp,
+ it returns the second of that timestamp.
+ If `expr` is a TIME type (since 4.1.0), it returns the second of the time-of-day.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_('2018-02-14 12:58:59');
+ 59
+ > SELECT _FUNC_(TIME'13:59:59.999999');
Review Comment:
@MaxGekk Could you please re-review the changes?
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #50525: [SPARK-51421][SQL] Get seconds of TIME datatype
URL: https://github.com/apache/spark/pull/50525
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2030323264
##########
sql/core/src/test/resources/sql-tests/results/hll.sql.out:
##########
@@ -66,9 +70,10 @@ SELECT hll_sketch_estimate(hll_union_agg(sketch, true))
SELECT hll_sketch_agg(col, 20) as sketch
FROM VALUES (1) AS tab(col))
-- !query schema
-struct<hll_sketch_estimate(hll_union_agg(sketch, true)):bigint>
+struct<>
-- !query output
-1
+java.lang.NoClassDefFoundError
+Could not initialize class org.apache.datasketches.memory.internal.BaseWritableMemoryImpl
Review Comment:
Seems like the output is invalid ..
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2032465033
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
Hi @vinodkc ,
MaxxGek has responded in jira that we need to return second without fraction. So as per Maxx requirement this PR will work without Fraction.
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2032475165
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
Review Comment:
@HyukjinKwon @vinodkc and @MaxGekk It will be helpful if you re-review this PR and provide your input
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50525:
URL: https://github.com/apache/spark/pull/50525#issuecomment-2789096890
@senthh Could you double check PR's description and replace `hour` by `second`, see
"
This PR adds support for extracting the **hour** component from TIME (TimeType) values in Spark SQL.
"
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "vinodkc (via GitHub)" <gi...@apache.org>.
vinodkc commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2030475356
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -168,4 +168,62 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkConsistencyBetweenInterpretedAndCodegen(
(child: Expression) => MinutesOfTime(child).replacement, TimeType())
}
+
+ test("SecondExpressionBuilder") {
+ // Empty expressions list
+ checkError(
+ exception = intercept[AnalysisException] {
+ SecondExpressionBuilder.build("second", Seq.empty)
+ },
+ condition = "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
+ parameters = Map(
+ "functionName" -> "`second`",
+ "expectedNum" -> "> 0",
+ "actualNum" -> "0",
+ "docroot" -> SPARK_DOC_ROOT)
+ )
+
+ // test TIME-typed child should build SecondsOfTime
+ val timeExpr = Literal(localTime(12, 58, 59), TimeType())
+ val builtExprForTime = SecondExpressionBuilder.build("second", Seq(timeExpr))
+ assert(builtExprForTime.isInstanceOf[SecondsOfTime])
+ assert(builtExprForTime.asInstanceOf[SecondsOfTime].child eq timeExpr)
+
+ // test non TIME-typed child should build second
+ val tsExpr = Literal("2007-09-03 10:45:23")
+ val builtExprForTs = SecondExpressionBuilder.build("second", Seq(tsExpr))
+ assert(builtExprForTs.isInstanceOf[Second])
+ assert(builtExprForTs.asInstanceOf[Second].child eq tsExpr)
+ }
+
+ test("Second with TIME type") {
+ // A few test times in microseconds since midnight:
+ // time in microseconds -> expected second
+ val testTimes = Seq(
Review Comment:
Can you please add this tests to test the output based on precision?
```
Seq(
0 -> 15.toDouble,
1 -> 15.9,
2 -> 15.98,
3 -> 15.987,
4 -> 15.9876,
5 -> 15.98765,
6 -> 15.987654).foreach { case (precision, expected) =>
checkEvaluation(
SecondsOfTimeWithFraction(Literal.create(time, TimeType(precision))),
BigDecimal(expected))
}
```
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #50525:
URL: https://github.com/apache/spark/pull/50525#issuecomment-2781628441
@MaxGekk Could you please review 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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2033214441
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+case class SecondsOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getSecondsOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
Now we are able to accept any precision of TimeType.
Query:
```
spark.sql("select second(cast('12:00:01.123' as time(3)))").show(false)
```
output:
```
+-------------------------------------+
|second(CAST(12:00:01.123 AS TIME(3)))|
+-------------------------------------+
|1 |
+-------------------------------------+
```
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #50525:
URL: https://github.com/apache/spark/pull/50525#issuecomment-2789389027
> @senthh Could you double check PR's description and replace `hour` by `second`, see " This PR adds support for extracting the **hour** component from TIME (TimeType) values in Spark SQL. "
@MaxGekk Yes Maxx, I have corrected PR description
--
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-51421][SQL] Get seconds of TIME datatype [spark]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #50525:
URL: https://github.com/apache/spark/pull/50525#issuecomment-2789392068
@HyukjinKwon Could you please re-review 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