MaxGekk commented on code in PR #52977:
URL: https://github.com/apache/spark/pull/52977#discussion_r3482750902
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala:
##########
@@ -708,6 +708,71 @@ trait SparkDateTimeUtils {
}
}
+ /**
+ * Returns a long corresponding to TIME (in nanos) constructed as the number
of seconds since
+ * 1970-01-01 00:00:00 UTC modulo 86400. Since Unix Epoch does not consider
leap seconds, this
+ * equates to the time at the number of seconds since midnight UTC.
Consequently, if the input
+ * long value is negative, the time is counted as absolute number of seconds
before midnight.
+ */
+ def integralToTime(seconds: Long, precision: Int): Option[Long] = {
+ try {
+ val SECONDS_PER_DAY = 24 * 60 * 60
+ val wrappedSeconds = Math.floorMod(seconds, SECONDS_PER_DAY)
+ val time: Long = LocalTime.ofSecondOfDay(wrappedSeconds).toNanoOfDay
+ Some(truncateTimeToPrecision(time, precision))
+ } catch {
+ case NonFatal(_) =>
+ None
+ }
+ }
+
+ /** ANSI version of the `integralToTime` method, throwing an error instead
of returning NULL. */
+ def integralToTimeAnsi(seconds: Long, precision: Int, context: QueryContext
= null): Long = {
+ integralToTime(seconds, precision).getOrElse {
+ throw ExecutionErrors.invalidInputInCastToDatetimeError(seconds,
TimeType(), context)
+ }
+ }
+
+ /**
+ * Returns a long corresponding to TIME (in nanos) constructed as the number
of seconds since
+ * 1970-01-01 00:00:00 UTC modulo 86400. Since Unix Epoch does not consider
leap seconds, this
+ * equates to the time at the number of seconds since midnight UTC.
Consequently, if the input
+ * long value is negative, the time is counted as absolute number of seconds
before midnight.
+ * Note that fractions smaller than nanoseconds are truncated, because max
TIME precision is 9.
+ */
+ def decimalToTime(d: Decimal, precision: Int): Option[Long] = {
+ integralToTime(d.toBigDecimal.longValue, precision)
Review Comment:
`d.toBigDecimal.longValue` truncates the input to a whole number of seconds
before conversion, so all fractional seconds are dropped — `CAST(1.5 AS TIME)`
→ `00:00:01`, not `00:00:01.5` (and `doubleToTime` has the same `d.toLong`).
This diverges from the numeric→TIMESTAMP analogue (`decimalToTimestamp` does
`(d.toBigDecimal * MICROS_PER_SECOND).longValue`, scaling *before* truncating),
and it defeats TIME's sub-second precision — the `precision` argument is
effectively dead on these paths since the value is already a whole second.
Suggest scaling seconds→nanos preserving the fraction (e.g. `value *
NANOS_PER_SECOND`, then `floorMod` by `NANOS_PER_DAY`), mirroring
`doubleToTimestamp`/`decimalToTimestamp`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala:
##########
@@ -1567,6 +1567,100 @@ abstract class CastSuiteBase extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("cast numeric to time") {
+ val fromTypes: Seq[DataType] = Seq(ByteType, ShortType, IntegerType,
LongType, DoubleType,
+ FloatType, DecimalType.USER_DEFAULT, DecimalType(10, 5),
DecimalType.SYSTEM_DEFAULT)
+ val toTypes: Seq[DataType] = Seq(TimeType(), TimeType(3), TimeType(6))
+ for {
+ fromType <- fromTypes
+ toType <- toTypes
+ } {
+ // Cast can be performed from `fromType` to `toType`.
+ assert(Cast.canCast(fromType, toType))
+ assert(Cast.canAnsiCast(fromType, toType))
+ }
+ // Test values.
+ val sec_0 = localTime()
+ val sec_1 = localTime(0, 0, 1)
+ val sec_100 = localTime(0, 1, 40)
+ val sec_999 = localTime(0, 16, 39)
+ val sec_10999 = localTime(3, 3, 19)
+ val sec_86399 = localTime(23, 59, 59)
+ val sec_86400 = sec_0
+ val sec_86401 = sec_1
+ val sec_172799 = sec_86399
+ val sec_1000999 = localTime(14, 3, 19)
+ val sec_2000000999 = localTime(3, 49, 59)
+ val sec_86399999999999 = sec_86399
+ // TINYINT
+ checkEvaluation(cast(Literal.create(0.toByte, ByteType), TimeType()),
sec_0)
+ checkEvaluation(cast(Literal.create(1.toByte, ByteType), TimeType()),
sec_1)
+ checkEvaluation(cast(Literal.create(100.toByte, ByteType), TimeType()),
sec_100)
+ // SMALLINT
+ checkEvaluation(cast(Literal.create(0.toShort, ShortType), TimeType()),
sec_0)
+ checkEvaluation(cast(Literal.create(1.toShort, ShortType), TimeType()),
sec_1)
+ checkEvaluation(cast(Literal.create(100.toShort, ShortType), TimeType()),
sec_100)
+ checkEvaluation(cast(Literal.create(999.toShort, ShortType), TimeType()),
sec_999)
+ checkEvaluation(cast(Literal.create(10999.toShort, ShortType),
TimeType()), sec_10999)
+ // INT
+ checkEvaluation(cast(Literal.create(0, IntegerType), TimeType()), sec_0)
+ checkEvaluation(cast(Literal.create(1, IntegerType), TimeType()), sec_1)
+ checkEvaluation(cast(Literal.create(100, IntegerType), TimeType()),
sec_100)
+ checkEvaluation(cast(Literal.create(999, IntegerType), TimeType()),
sec_999)
+ checkEvaluation(cast(Literal.create(10999, IntegerType), TimeType()),
sec_10999)
+ checkEvaluation(cast(Literal.create(86399, IntegerType), TimeType()),
sec_86399)
+ checkEvaluation(cast(Literal.create(86400, IntegerType), TimeType()),
sec_86400)
+ checkEvaluation(cast(Literal.create(86401, IntegerType), TimeType()),
sec_86401)
+ checkEvaluation(cast(Literal.create(172799, IntegerType), TimeType()),
sec_172799)
+ checkEvaluation(cast(Literal.create(1000999, IntegerType), TimeType()),
sec_1000999)
+ checkEvaluation(cast(Literal.create(2000000999, IntegerType), TimeType()),
sec_2000000999)
+ // BIGINT
+ checkEvaluation(cast(Literal.create(0L, LongType), TimeType()), sec_0)
+ checkEvaluation(cast(Literal.create(1L, LongType), TimeType()), sec_1)
+ checkEvaluation(cast(Literal.create(100L, LongType), TimeType()), sec_100)
+ checkEvaluation(cast(Literal.create(999L, LongType), TimeType()), sec_999)
+ checkEvaluation(cast(Literal.create(10999L, LongType), TimeType()),
sec_10999)
+ checkEvaluation(cast(Literal.create(86399L, LongType), TimeType()),
sec_86399)
+ checkEvaluation(cast(Literal.create(86400L, LongType), TimeType()),
sec_86400)
+ checkEvaluation(cast(Literal.create(86401L, LongType), TimeType()),
sec_86401)
+ checkEvaluation(cast(Literal.create(172799L, LongType), TimeType()),
sec_172799)
+ checkEvaluation(cast(Literal.create(1000999L, LongType), TimeType()),
sec_1000999)
+ checkEvaluation(cast(Literal.create(2000000999L, LongType), TimeType()),
sec_2000000999)
+ checkEvaluation(cast(Literal.create(86399999999999L, LongType),
TimeType()), sec_86399999999999)
+ // DECIMAL
+ checkEvaluation(cast(Literal.create(Decimal(0)), TimeType()), sec_0)
+ checkEvaluation(cast(Literal.create(Decimal(1)), TimeType()), sec_1)
+ checkEvaluation(cast(Literal.create(Decimal(100)), TimeType()), sec_100)
+ checkEvaluation(cast(Literal.create(Decimal(999)), TimeType()), sec_999)
+ checkEvaluation(cast(Literal.create(Decimal(10999)), TimeType()),
sec_10999)
+ checkEvaluation(cast(Literal.create(Decimal(86399)), TimeType()),
sec_86399)
Review Comment:
All the Decimal/Double/Float cases use whole numbers (`Decimal(86399)`,
`1.0`, `100.0`, …), so they're indistinguishable from the integral cases and
never exercise the one thing that's distinct about these types —
fractional-second handling. As a result the truncation issue on
`decimalToTime`/`doubleToTime` is invisible to the suite. Worth adding a
fractional case (e.g. `CAST(1.5 AS TIME(1))` → `00:00:01.5`, once the scaling
is fixed) and a NaN/Infinity Double case (the only path that actually returns
null / errors).
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala:
##########
@@ -708,6 +708,71 @@ trait SparkDateTimeUtils {
}
}
+ /**
+ * Returns a long corresponding to TIME (in nanos) constructed as the number
of seconds since
+ * 1970-01-01 00:00:00 UTC modulo 86400. Since Unix Epoch does not consider
leap seconds, this
+ * equates to the time at the number of seconds since midnight UTC.
Consequently, if the input
+ * long value is negative, the time is counted as absolute number of seconds
before midnight.
+ */
+ def integralToTime(seconds: Long, precision: Int): Option[Long] = {
+ try {
+ val SECONDS_PER_DAY = 24 * 60 * 60
+ val wrappedSeconds = Math.floorMod(seconds, SECONDS_PER_DAY)
+ val time: Long = LocalTime.ofSecondOfDay(wrappedSeconds).toNanoOfDay
+ Some(truncateTimeToPrecision(time, precision))
+ } catch {
+ case NonFatal(_) =>
+ None
+ }
+ }
+
+ /** ANSI version of the `integralToTime` method, throwing an error instead
of returning NULL. */
+ def integralToTimeAnsi(seconds: Long, precision: Int, context: QueryContext
= null): Long = {
+ integralToTime(seconds, precision).getOrElse {
+ throw ExecutionErrors.invalidInputInCastToDatetimeError(seconds,
TimeType(), context)
Review Comment:
Minor: `integralToTime` can never return `None` — `Math.floorMod(seconds,
86400)` is always in `[0, 86399]`, and
`ofSecondOfDay`/`toNanoOfDay`/`truncateTimeToPrecision` all succeed for that
range — so this `getOrElse`-throw (and the new
`invalidInputInCastToDatetimeError(value: Long, …)` overload) is unreachable
for integral and decimal inputs. Only `doubleToTime` (NaN/Infinity, handled
before this call) actually yields `None`. Consider dropping the `Option`/try on
the integral path, or a comment noting why it's kept.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala:
##########
@@ -708,6 +708,71 @@ trait SparkDateTimeUtils {
}
}
+ /**
+ * Returns a long corresponding to TIME (in nanos) constructed as the number
of seconds since
+ * 1970-01-01 00:00:00 UTC modulo 86400. Since Unix Epoch does not consider
leap seconds, this
+ * equates to the time at the number of seconds since midnight UTC.
Consequently, if the input
+ * long value is negative, the time is counted as absolute number of seconds
before midnight.
+ */
+ def integralToTime(seconds: Long, precision: Int): Option[Long] = {
+ try {
+ val SECONDS_PER_DAY = 24 * 60 * 60
+ val wrappedSeconds = Math.floorMod(seconds, SECONDS_PER_DAY)
+ val time: Long = LocalTime.ofSecondOfDay(wrappedSeconds).toNanoOfDay
+ Some(truncateTimeToPrecision(time, precision))
+ } catch {
+ case NonFatal(_) =>
+ None
+ }
+ }
+
+ /** ANSI version of the `integralToTime` method, throwing an error instead
of returning NULL. */
+ def integralToTimeAnsi(seconds: Long, precision: Int, context: QueryContext
= null): Long = {
+ integralToTime(seconds, precision).getOrElse {
+ throw ExecutionErrors.invalidInputInCastToDatetimeError(seconds,
TimeType(), context)
+ }
+ }
+
+ /**
+ * Returns a long corresponding to TIME (in nanos) constructed as the number
of seconds since
+ * 1970-01-01 00:00:00 UTC modulo 86400. Since Unix Epoch does not consider
leap seconds, this
+ * equates to the time at the number of seconds since midnight UTC.
Consequently, if the input
+ * long value is negative, the time is counted as absolute number of seconds
before midnight.
+ * Note that fractions smaller than nanoseconds are truncated, because max
TIME precision is 9.
Review Comment:
Nit: this Scaladoc (and the `doubleToTime` block) is copied from
`integralToTime` — it says "if the input **long** value is negative" though the
params here are `Decimal`/`Double`, and it claims "fractions smaller than
nanoseconds are truncated", but the code truncates *all* sub-second fractions
(via `longValue`/`toLong`), not just sub-nanosecond. Worth fixing the wording
and the truncation claim (the latter becomes accurate once the scaling
preserves the 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
