MaxGekk commented on code in PR #59955:
URL: https://github.com/apache/spark/pull/59955#discussion_r4424323544
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1105,6 +1105,40 @@ object DateTimeUtils extends SparkDateTimeUtils {
}
}
+ /** The whole seconds and the nanos of the second of a seconds value with a
fraction. */
+ case class SecondsAndNanos(seconds: Int, nanos: Int)
+
+ /**
+ * Splits the seconds of a minute with a fraction of 6 digits (microseconds)
into the whole
+ * seconds and the nanos of the second, as `LocalTime.of` and
`LocalDateTime.of` take them.
+ *
+ * The whole seconds are checked to be in the int range and not negative
before they are
+ * narrowed. A value outside of the int range would overflow when converted
from long to int,
+ * and could produce an int in the valid seconds range and return a wrong
value, or a misleading
+ * error message. A negative value is rejected here for a better error
message: `Math.floorDiv`
+ * gets the next lower integer for it, so the full seconds would not match
the value given.
+ * The message is the one that `LocalTime.of` and `LocalDateTime.of` throw
for invalid seconds,
+ * with the seconds given, truncated towards zero, or in full for a value
between -1 and 0,
+ * which the truncation would show as the valid 0. Any other value, such as
61 seconds, is
+ * left to them, which check their fields in order, so an invalid date or
hour is reported
+ * before such seconds, while a negative value or one above `Int.MaxValue`
is reported first.
+ */
+ def splitSecsAndMicros(secsAndMicros: Decimal): SecondsAndNanos = {
+ val unscaledSecFrac = secsAndMicros.toUnscaledLong
+ val fullSecs = Math.floorDiv(unscaledSecFrac, MICROS_PER_SECOND)
+ if (fullSecs > Int.MaxValue || fullSecs < 0) {
Review Comment:
Thanks for the table. I would keep the new order: it is the one `make_time`
has had since the negative check was added there, and sharing the helper means
`make_timestamp` and `make_time` now report the same seconds for the same
input, which was the point of the shared split (the floored `-1` for `-0.5`, or
`-4265` for `-4264.967296`, is what the earlier review objected to). Letting
in-range negative values through to `LocalDateTime.of` would bring that
difference back for every negative value. So the order is: a negative value or
one above `Int.MaxValue` first, everything else in field order. Pinned by your
cases from item 2 in 9c4ab6490a7, and the user-facing section of the
description says so.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -1599,6 +1599,55 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-60743: make_timestamp rejects seconds whose micros do not fit an
int") {
+ // The seconds input is DecimalType(16, 6), so its unscaled micros can
exceed an int.
+ // Each value below must fail like 61 seconds does instead of wrapping
modulo 2^32. The
+ // error shows the seconds given, truncated towards zero, as make_time
reports them, or in
+ // full for a value between -1 and 0, which the truncation would show as
the valid 0.
+ val outOfRangeSeconds = Seq(
Review Comment:
Added your four cases with `month = 13` in 9c4ab6490a7: `-1` and
`2147483648` report `SecondOfMinute` first, `61` and `2147483647` report
`MonthOfYear`, with ANSI on, and NULL with ANSI off.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -1599,6 +1599,55 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-60743: make_timestamp rejects seconds whose micros do not fit an
int") {
+ // The seconds input is DecimalType(16, 6), so its unscaled micros can
exceed an int.
+ // Each value below must fail like 61 seconds does instead of wrapping
modulo 2^32. The
+ // error shows the seconds given, truncated towards zero, as make_time
reports them, or in
+ // full for a value between -1 and 0, which the truncation would show as
the valid 0.
+ val outOfRangeSeconds = Seq(
+ // (year, month, day, hour, min, sec, the seconds shown in the error)
+ // The largest micros that fit an int, and the first that do not.
+ (2020, 1, 1, 0, 0, "2147.483647", "2147"),
+ (2020, 1, 1, 0, 0, "2147.483648", "2147"),
+ // 4324967296 - 2^32 = 30000000 micros, which would read as 30 seconds.
+ (2020, 1, 1, 0, 0, "4324.967296", "4324"),
+ // 4354967296 - 2^32 = 60000000 micros, which would roll over to the
next minute.
+ (2019, 6, 30, 23, 59, "4354.967296", "4354"),
Review Comment:
Right, `4354.967297` wrapped to 60.000001 seconds and raised
`INVALID_FRACTION_OF_SECOND` with the value as given; now it is out of range
like 61 seconds and raises `DATETIME_FIELD_OUT_OF_BOUNDS.WITHOUT_SUGGESTION`
with `4354`. Added the value to the table in 9c4ab6490a7, and the user-facing
section of the description mentions the change of error class for a value that
wrapped into the fraction of the 60th second.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -1599,6 +1599,55 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-60743: make_timestamp rejects seconds whose micros do not fit an
int") {
+ // The seconds input is DecimalType(16, 6), so its unscaled micros can
exceed an int.
+ // Each value below must fail like 61 seconds does instead of wrapping
modulo 2^32. The
+ // error shows the seconds given, truncated towards zero, as make_time
reports them, or in
+ // full for a value between -1 and 0, which the truncation would show as
the valid 0.
+ val outOfRangeSeconds = Seq(
+ // (year, month, day, hour, min, sec, the seconds shown in the error)
+ // The largest micros that fit an int, and the first that do not.
+ (2020, 1, 1, 0, 0, "2147.483647", "2147"),
+ (2020, 1, 1, 0, 0, "2147.483648", "2147"),
+ // 4324967296 - 2^32 = 30000000 micros, which would read as 30 seconds.
+ (2020, 1, 1, 0, 0, "4324.967296", "4324"),
+ // 4354967296 - 2^32 = 60000000 micros, which would roll over to the
next minute.
+ (2019, 6, 30, 23, 59, "4354.967296", "4354"),
+ // 3000000000 micros would wrap to a negative int.
+ (2020, 1, 1, 0, 0, "3000", "3000"),
+ // A negative value whose micros would wrap to 30 seconds.
+ (2020, 1, 1, 0, 0, "-4264.967296", "-4264"),
+ // Values between -1 and 0, shown in full.
+ (2020, 1, 1, 0, 0, "-0.5", "-0.5"),
+ (2020, 1, 1, 0, 0, "-0.000001", "-0.000001"),
+ // The largest and the smallest DecimalType(16, 6) values: even their
whole seconds do
+ // not fit an int.
+ (2020, 1, 1, 0, 0, "9999999999.999999", "9999999999"),
+ (2020, 1, 1, 0, 0, "-9999999999.999999", "-9999999999"),
+ // A small out-of-range value, for comparison.
+ (2020, 1, 1, 0, 0, "61", "61"))
+ for {
+ dataType <- Seq(TimestampType, TimestampNTZType)
+ ansi <- Seq(true, false)
+ (year, month, day, hour, min, sec, shown) <- outOfRangeSeconds
+ } {
+ val expr = MakeTimestamp(Literal(year), Literal(month), Literal(day),
Literal(hour),
+ Literal(min), Literal(Decimal(BigDecimal(sec), 16, 6)), timeZoneId =
Some("UTC"),
Review Comment:
Added a `timezone = Some(Literal("UTC"))` variant of the expression test for
`TimestampType`, and one out-of-range case next to the existing six-argument
calls in `timestamp-ltz.sql` and `timestamp-ntz.sql`, with their golden files
regenerated, in 9c4ab6490a7.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -1599,6 +1599,55 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-60743: make_timestamp rejects seconds whose micros do not fit an
int") {
Review Comment:
Took the suggestion in 9c4ab6490a7.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/DateTimeExpressionUtils.java:
##########
@@ -96,12 +95,12 @@ public static long makeTimestampMicros(
Decimal secAndMicros, ZoneId zoneId, boolean timestampNTZ) {
assert secAndMicros.scale() == 6 :
"Seconds fraction must have 6 digits for microseconds but got " +
secAndMicros.scale();
- // 8 digits cannot overflow Int.
- int totalMicros = (int) secAndMicros.toUnscaledLong();
- int microsPerSecond = (int) DateTimeConstants.MICROS_PER_SECOND;
- int nanosPerMicros = (int) DateTimeConstants.NANOS_PER_MICROS;
- int seconds = Math.floorDiv(totalMicros, microsPerSecond);
- int nanos = Math.floorMod(totalMicros, microsPerSecond) * nanosPerMicros;
+ // The seconds are DecimalType(16, 6), so their whole seconds can exceed
an int. The split
+ // rejects such seconds, and negative ones, before narrowing, as make_time
does; see
+ // DateTimeUtils.splitSecsAndMicros for the error message and the order of
the checks.
Review Comment:
Updated in 9c4ab6490a7: the Javadoc now says that a `DateTimeException` is
thrown for seconds that are negative or above `Integer.MAX_VALUE`, before the
other fields are checked, or for an invalid combination of the other fields
otherwise, 61 seconds included.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/DateTimeExpressionUtils.java:
##########
@@ -96,12 +95,12 @@ public static long makeTimestampMicros(
Decimal secAndMicros, ZoneId zoneId, boolean timestampNTZ) {
assert secAndMicros.scale() == 6 :
Review Comment:
Moved into `splitSecsAndMicros` as a plain `assert(secsAndMicros.scale ==
6)` in 9c4ab6490a7, so `make_time` is covered too, and dropped from
`makeTimestampMicros`.
--
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]
