You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "the-sakthi (via GitHub)" <gi...@apache.org> on 2025/03/17 11:37:56 UTC
[PR] [SPARK-51420][SQL] Get minutes of TIME datatype [spark]
the-sakthi opened a new pull request, #50296:
URL: https://github.com/apache/spark/pull/50296
### What changes were proposed in this pull request?
This PR adds support for extracting the minute component from TIME (TimeType) values in Spark SQL.
``` bash
scala> spark.sql("SELECT minute(TIME'12:58:59');").show()
+-------------------+
|minute(46739000000)|
+-------------------+
| 58|
+-------------------+
```
### Why are the changes needed?
- Spark previously supported minute() for only TIMESTAMP type values.
- TIME support was missing, leading to implicit casting attempt to TIMESTAMP, which was incorrect.
- This PR ensures that `minute(TIME'HH:MM:SS.######')` behaves correctly without unnecessary type coercion.
### Does this PR introduce _any_ user-facing change?
Yes
- Before this PR, calling `minute(TIME'HH:MM:SS.######')` resulted in a type mismatch error or an implicit cast attempt to TIMESTAMP, which was incorrect.
- With this PR, `minute(TIME'HH:MM:SS.######')` now works correctly for TIME values without implicit casting.
- Users can now extract the minute component from TIME values natively.
### How was this patch tested?
By running new tests:
```
$ build/sbt "test:testOnly *DateExpressionsSuite"
```
### 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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2001066777
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +121,53 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'12:58:59');
+ 58
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
Review Comment:
Note: The codegen issue pops up while running the added UT when it reaches `checkConsistencyBetweenInterpretedAndCodegen`.
spark-shell based manual testing passed fine, so I'm assuming that it chose the interpreter path over there.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2004840330
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -18,12 +18,13 @@
package org.apache.spark.sql.catalyst.expressions
import java.time.DateTimeException
-
-import org.apache.spark.sql.catalyst.expressions.objects.Invoke
+import org.apache.spark.sql.catalyst.analysis.ExpressionBuilder
+import org.apache.spark.sql.catalyst.expressions.objects.{Invoke, StaticInvoke}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.catalyst.util.TimeFormatter
import org.apache.spark.sql.errors.QueryExecutionErrors
import org.apache.spark.sql.internal.types.StringTypeWithCollation
-import org.apache.spark.sql.types.{AbstractDataType, ObjectType, TimeType}
+import org.apache.spark.sql.types.{AbstractDataType, IntegerType, LongType, ObjectType, TimeType}
Review Comment:
Yes, had that import since I am not yet sure if we are to explicitly cast to LongType before staticInvoking.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2001062893
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +121,53 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'12:58:59');
+ 58
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
Review Comment:
I'm running into code generation issues here. For e.g: `Cannot generate code for expression: minutes_of_time(13:52:57.421543)`
I tried updating the StaticInvoke call to use `Seq(Cast(child, LongType))` and `Seq(LongType)` in replacement, but I'm still seeing failures in the codegen path. Any suggestions on how to properly handle the TIME → LONG cast for codegen, or if there's a simpler workaround, Max?
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2729685404
> I’ll keep using the existing Minute expression (or rename it to MinuteForTimestamp).
@the-sakthi Let's leave it as is.
> Then, in the function registry, I’ll dispatch to either MinuteForTime or MinuteForTimestamp based on the input data type.
Could you introduce new expression `MinutesOfTime` and place it to `timeExpressions.scala` (see https://github.com/apache/spark/pull/50287). `MinuteOfTime` should extend `RuntimeReplaceable` and use `Invoke`. The last one just invokes `getMinutesOfTime` from `DateTimeUtils`.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005023753
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala:
##########
@@ -123,6 +124,18 @@ object LiteralGenerator {
yield Literal.create(new Date(day * MILLIS_PER_DAY), DateType)
}
+ private def timeMicrosGen: Gen[Long] = {
+ // Generate a random time represented as microseconds since midnight.
+ val minMicros = localTime() // 00:00:00.000000
+ val maxMicros = localTime(23, 59, 59, 999999) // 23:59:59.999999 in microseconds
+ Gen.choose(minMicros, maxMicros)
+ }
+
+ lazy val timeTypeLiteralGen: Gen[Literal] = {
+ for { micros <- timeMicrosGen }
+ yield Literal.create(micros, TimeType(6)) // Use microsecond precision
+ }
Review Comment:
Ahh didn't notice the separate JIRA. I'll cater this change for that jira then.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2004831936
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -18,12 +18,13 @@
package org.apache.spark.sql.catalyst.expressions
import java.time.DateTimeException
-
Review Comment:
Please, add a blank line.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -18,12 +18,13 @@
package org.apache.spark.sql.catalyst.expressions
import java.time.DateTimeException
-
-import org.apache.spark.sql.catalyst.expressions.objects.Invoke
+import org.apache.spark.sql.catalyst.analysis.ExpressionBuilder
+import org.apache.spark.sql.catalyst.expressions.objects.{Invoke, StaticInvoke}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.catalyst.util.TimeFormatter
import org.apache.spark.sql.errors.QueryExecutionErrors
import org.apache.spark.sql.internal.types.StringTypeWithCollation
-import org.apache.spark.sql.types.{AbstractDataType, ObjectType, TimeType}
+import org.apache.spark.sql.types.{AbstractDataType, IntegerType, LongType, ObjectType, TimeType}
Review Comment:
Could you fix the error:
```
[ERROR] [Error] /Users/maxim.gekk/proj/review-the-sakthi_SPARK-51420/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:27: Unused import
Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=unused-imports, site=org.apache.spark.sql.catalyst.expressions
```
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2729180913
@MaxGekk looking forward to know your thoughts on this one!
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2729444777
Thanks for the review @MaxGekk
That totally makes sense. I’ll introduce a separate `MinuteForTime` expression for TIME inputs that doesn’t depend on `GetTimeField` or time zones. For TIMESTAMP inputs, I’ll keep using the existing Minute expression (or rename it to MinuteForTimestamp). Then, in the function registry, I’ll dispatch to either `MinuteForTime` or `MinuteForTimestamp` based on the input data type. Let me know if that doesn’t align with what you had in mind.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005019421
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala:
##########
@@ -123,6 +124,18 @@ object LiteralGenerator {
yield Literal.create(new Date(day * MILLIS_PER_DAY), DateType)
}
+ private def timeMicrosGen: Gen[Long] = {
+ // Generate a random time represented as microseconds since midnight.
+ val minMicros = localTime() // 00:00:00.000000
+ val maxMicros = localTime(23, 59, 59, 999999) // 23:59:59.999999 in microseconds
+ Gen.choose(minMicros, maxMicros)
+ }
+
+ lazy val timeTypeLiteralGen: Gen[Literal] = {
+ for { micros <- timeMicrosGen }
+ yield Literal.create(micros, TimeType(6)) // Use microsecond precision
+ }
Review Comment:
There is a separate task for this: https://issues.apache.org/jira/browse/SPARK-51455. How about to implement it in a separate PR, and test the existing expression: `ToTime`?
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2739524486
@the-sakthi Could you update PR's description:
```
scala> spark.sql("SELECT minute(TIME'12:58:59');").show()
+-------------------+
|minute(46739000000)|
```
This PR https://github.com/apache/spark/pull/50299 fixed the column name.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r1998660187
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -388,6 +388,44 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
}
}
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val secondsPerHour = 3600L
+ val secondsPerMinute = 60L
+ val testTimes = Seq(
+ (0L, 0), // 00:00:00 => 0
+ ((12L * secondsPerHour + 58L * secondsPerMinute + 59L) * 1000000L, 58), // 12:58:59 => 58
Review Comment:
Sure.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #50296: [SPARK-51420][SQL] Get minutes of TIME datatype
URL: https://github.com/apache/spark/pull/50296
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2036334022
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -160,3 +161,75 @@ object TryToTimeExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
@MaxGekk Created the followup PR: https://github.com/apache/spark/pull/50551 for the fix.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005017827
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +121,53 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'12:58:59');
+ 58
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minutes_of_time"
Review Comment:
should be `minute`. You are going to bind `MinuteExpressionBuilder` to `minute`, right?
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2035711415
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -160,3 +161,75 @@ object TryToTimeExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
sure let me take care of this, Max
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2739735648
Updated the revision with the suggested changes @MaxGekk ! Let me know how this one looks.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005021763
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -51,4 +51,37 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
condition = "CANNOT_PARSE_TIME",
parameters = Map("input" -> "'100:50'", "format" -> "'mm:HH'"))
}
+
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val testTimes = Seq(
+ (localTime(), 0), // 00:00:00 => 0
+ (localTime(12, 58, 59), 58), // 12:58:59 => 58
+ (localTime(0, 59), 59), // 00:59:00 => 59
+ (localTime(23, 0, 1), 0), // 23:00:01 => 0
+ (localTime(1), 0), // 01:00:00 => 0
+ (localTime(14, 30), 30), // 14:30:00 => 30
+ (localTime(23, 59, 59, 999999), 59) // 23:59:59.999999 => 59
Review Comment:
The comments look useless. Just do that in the code:
```suggestion
localTime(23, 59, 59, 999999) -> 59
```
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2008662432
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +122,52 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minute"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+object MinuteExpressionBuilder extends ExpressionBuilder {
+ override def build(name: String, expressions: Seq[Expression]): Expression = {
+ val child = expressions.head
Review Comment:
Fixed the updated revision. Thanks for catching this, Max.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2035301942
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -160,3 +161,75 @@ object TryToTimeExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
@the-sakthi Could you open a follow up PR and allow any precision of the TIME type as you did for `second()` otherwise it fails now:
```sql
spark-sql (default)> select minute(cast('12:30' as time(0)));
[DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE] Cannot resolve "minute(CAST(12:30 AS TIME(0)))" due to data type mismatch: The first parameter requires the "TIME(6)" type, however "CAST(12:30 AS TIME(0))" has the type "TIME(0)". SQLSTATE: 42K09; line 1 pos 7;
```
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005167278
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +121,53 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'12:58:59');
+ 58
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
Review Comment:
Ignoring this issue since the `checkConsistencyBetweenInterpretedAndCodegen` isn't likely to be tested like this when using StaticInvoke
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005402572
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +122,52 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minute"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
Review Comment:
Moved the function usage/desc/example to the builder to fix a failing test which was complaining about this.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r1998572198
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -388,6 +388,44 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
}
}
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val secondsPerHour = 3600L
+ val secondsPerMinute = 60L
+ val testTimes = Seq(
+ (0L, 0), // 00:00:00 => 0
+ ((12L * secondsPerHour + 58L * secondsPerMinute + 59L) * 1000000L, 58), // 12:58:59 => 58
+ ((0L * secondsPerHour + 59L * secondsPerMinute + 0L) * 1000000L, 59), // 00:59:00 => 59
+ ((23L * secondsPerHour + 0L * secondsPerMinute + 1L) * 1000000L, 0), // 23:00:01 => 0
+ ((1L * secondsPerHour) * 1000000L, 0), // 01:00:00 => 0
+ ((14L * secondsPerHour + 30L * secondsPerMinute) * 1000000L, 30), // 14:30:00 => 30
+ ((23L * secondsPerHour + 59L * secondsPerMinute + 59L) * 1000000L
+ + 999999L, 59) // 23:59:59.999999 => 59
+ )
+
+ // Create a literal with TimeType() for each test microsecond value
+ // evaluate Minute(...), and check that the result matches the expected minute.
+ testTimes.foreach { case (micros, expectedMinute) =>
+ checkEvaluation(
+ Minute(Literal(micros, TimeType())),
+ expectedMinute)
+ }
+
+ // Verify NULL handling
+ checkEvaluation(
+ Minute(Literal.create(null, TimeType(6))),
+ null
+ )
+
+ // Verify that the expression is consistent in interpreted vs. codegen mode.
+ checkConsistencyBetweenInterpretedAndCodegen(
+ (child: Expression) => Minute(child), TimeType())
+ }
+
+
+
+
Review Comment:
Please, remove the blank lines.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -388,6 +388,44 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
}
}
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val secondsPerHour = 3600L
+ val secondsPerMinute = 60L
+ val testTimes = Seq(
+ (0L, 0), // 00:00:00 => 0
+ ((12L * secondsPerHour + 58L * secondsPerMinute + 59L) * 1000000L, 58), // 12:58:59 => 58
Review Comment:
You can use the helper function `localTime` to get micros.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -429,8 +436,18 @@ case class Hour(child: Expression, timeZoneId: Option[String] = None) extends Ge
case class Minute(child: Expression, timeZoneId: Option[String] = None) extends GetTimeField {
Review Comment:
`GetTimeField` is time zone aware expression, but in the case of TIME, `Minute` shouldn't depend on time zone, and require time zone resolution.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -113,6 +113,15 @@ object DateTimeUtils extends SparkDateTimeUtils {
getLocalDateTime(micros, zoneId).getMinute
}
+ /**
+ * Returns the minute value of a given TIME (TimeType) value.
+ * The `ZoneId` parameter is ignored because TIME values do not require time zones.
+ * It is included only to satisfy the `GetTimeField` function signature.
+ */
+ def getMinutesFromTime(micros: Long, zoneId: ZoneId): Int = {
+ SparkDateTimeUtils.microsToLocalTime(micros).getMinute
Review Comment:
`microsToLocalTime` should be in the scope:
```suggestion
microsToLocalTime(micros).getMinute
```
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2745374919
Thank you very much for helping on this one and merging @MaxGekk
Much appreciated!
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2039182055
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -160,3 +161,75 @@ object TryToTimeExpressionBuilder extends ExpressionBuilder {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
Review Comment:
Followup PR merged. resolving this comment.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005022335
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -51,4 +51,37 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
condition = "CANNOT_PARSE_TIME",
parameters = Map("input" -> "'100:50'", "format" -> "'mm:HH'"))
}
+
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val testTimes = Seq(
+ (localTime(), 0), // 00:00:00 => 0
+ (localTime(12, 58, 59), 58), // 12:58:59 => 58
+ (localTime(0, 59), 59), // 00:59:00 => 59
+ (localTime(23, 0, 1), 0), // 23:00:01 => 0
+ (localTime(1), 0), // 01:00:00 => 0
+ (localTime(14, 30), 30), // 14:30:00 => 30
+ (localTime(23, 59, 59, 999999), 59) // 23:59:59.999999 => 59
+ )
+
+ // Create a literal with TimeType() for each test microsecond value
+ // evaluate Minute(...), and check that the result matches the expected minute.
+ testTimes.foreach { case (micros, expectedMinute) =>
+ checkEvaluation(
+ MinutesOfTime(Literal(micros, TimeType())),
+ expectedMinute)
+ }
+
+ // Verify NULL handling
+ checkEvaluation(
+ MinutesOfTime(Literal.create(null, TimeType(6))),
+ null
+ )
+
+ // Verify that the expression is consistent in interpreted vs. codegen mode.
+ // TODO: codegen failing. Pending fix
+ checkConsistencyBetweenInterpretedAndCodegen(
+ (child: Expression) => MinutesOfTime(child), TimeType())
+ }
Review Comment:
I think this part of the testing code isn't how this is supposed to be tested. I'll remove this from the next version of the PR, if that's fine.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005025373
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +121,53 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'12:58:59');
+ 58
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minutes_of_time"
Review Comment:
Ah yes! This one slipped through my debugging/testing. Will modify.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2739542633
Thanks for the review @MaxGekk , I'll shortly in few mins update the PR. Major changes:
- Will remove the LiteralGenerator part of the change for a separate PR
- Will remove the `checkConsistencyBetweenInterpretedAndCodegen` part of the testing.
- Will cater to other review comments and update the 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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2006853665
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +122,52 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minute"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+object MinuteExpressionBuilder extends ExpressionBuilder {
+ override def build(name: String, expressions: Seq[Expression]): Expression = {
+ val child = expressions.head
Review Comment:
Please, handle empty `expressions`. For now, it fails with:
```sql
spark-sql (default)> SELECT minute();
25/03/21 07:56:16 ERROR SparkSQLDriver: Failed in [SELECT minute()]
java.util.NoSuchElementException: head of empty list
at scala.collection.immutable.Nil$.head(List.scala:663)
at scala.collection.immutable.Nil$.head(List.scala:662)
at org.apache.spark.sql.catalyst.expressions.MinuteExpressionBuilder$.build(timeExpressions.scala:165)
```
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2745334574
+1, LGTM. Merging to master.
Thank you, @the-sakthi.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2733280959
Let me know if this updated PR aligns better with your suggestions, @MaxGekk
Also, I'm looking for guidance, if possible, on fixing the codegen issue which I pointed above.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r1998662328
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -429,8 +436,18 @@ case class Hour(child: Expression, timeZoneId: Option[String] = None) extends Ge
case class Minute(child: Expression, timeZoneId: Option[String] = None) extends GetTimeField {
Review Comment:
Makes 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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2008652295
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -120,3 +122,52 @@ case class ToTimeParser(fmt: Option[String]) {
}
}
}
+
+case class MinutesOfTime(child: Expression)
+ extends RuntimeReplaceable
+ with ExpectsInputTypes {
+
+ override def replacement: Expression = StaticInvoke(
+ classOf[DateTimeUtils.type],
+ IntegerType,
+ "getMinutesOfTime",
+ Seq(child),
+ Seq(child.dataType)
+ )
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+ override def children: Seq[Expression] = Seq(child)
+
+ override def prettyName: String = "minute"
+
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression = {
+ copy(child = newChildren.head)
+ }
+}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = """
+ _FUNC_(time_expr) - Returns the minute component of the given time.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'23:59:59.999999');
+ 59
+ """,
+ since = "4.1.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+object MinuteExpressionBuilder extends ExpressionBuilder {
+ override def build(name: String, expressions: Seq[Expression]): Expression = {
+ val child = expressions.head
Review Comment:
Ahh my bad! Should have handled this preemptively. Will fix this.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on PR #50296:
URL: https://github.com/apache/spark/pull/50296#issuecomment-2745028061
Rebased with current main (master) branch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005169204
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -51,4 +51,37 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
condition = "CANNOT_PARSE_TIME",
parameters = Map("input" -> "'100:50'", "format" -> "'mm:HH'"))
}
+
+ test("Minute with TIME type") {
+ // A few test times in microseconds since midnight:
+ // (time in microseconds, expected minute)
+ val testTimes = Seq(
+ (localTime(), 0), // 00:00:00 => 0
+ (localTime(12, 58, 59), 58), // 12:58:59 => 58
+ (localTime(0, 59), 59), // 00:59:00 => 59
+ (localTime(23, 0, 1), 0), // 23:00:01 => 0
+ (localTime(1), 0), // 01:00:00 => 0
+ (localTime(14, 30), 30), // 14:30:00 => 30
+ (localTime(23, 59, 59, 999999), 59) // 23:59:59.999999 => 59
+ )
+
+ // Create a literal with TimeType() for each test microsecond value
+ // evaluate Minute(...), and check that the result matches the expected minute.
+ testTimes.foreach { case (micros, expectedMinute) =>
+ checkEvaluation(
+ MinutesOfTime(Literal(micros, TimeType())),
+ expectedMinute)
+ }
+
+ // Verify NULL handling
+ checkEvaluation(
+ MinutesOfTime(Literal.create(null, TimeType(6))),
+ null
+ )
+
+ // Verify that the expression is consistent in interpreted vs. codegen mode.
+ // TODO: codegen failing. Pending fix
+ checkConsistencyBetweenInterpretedAndCodegen(
+ (child: Expression) => MinutesOfTime(child), TimeType())
+ }
Review Comment:
Removed this code from the next revision.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "the-sakthi (via GitHub)" <gi...@apache.org>.
the-sakthi commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2004840730
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -18,12 +18,13 @@
package org.apache.spark.sql.catalyst.expressions
import java.time.DateTimeException
-
Review Comment:
Yes will do.
--
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-51420][SQL] Get minutes of TIME datatype [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50296:
URL: https://github.com/apache/spark/pull/50296#discussion_r2005020437
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala:
##########
@@ -196,6 +209,7 @@ object LiteralGenerator {
case DoubleType => doubleLiteralGen
case FloatType => floatLiteralGen
case DateType => dateLiteralGen
+ case TimeType(6) => timeTypeLiteralGen
Review Comment:
`TimeType()` or use a constant: `TimeType.MICROS_PRECISION`
--
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