dongjoon-hyun commented on code in PR #59955:
URL: https://github.com/apache/spark/pull/59955#discussion_r4424175172
##########
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:
**3. [Low] A value that wrapped to just above 60 seconds changes its error
class**
Next to this case, `4354.967297` wrapped to 60.000001 seconds before this
PR. With ANSI on it raised `INVALID_FRACTION_OF_SECOND` with the value as given
(`4354.967297`). It now raises
`DATETIME_FIELD_OUT_OF_BOUNDS.WITHOUT_SUGGESTION` with `4354`.
The new result is right and matches 61 seconds, but it is a different error
class for anyone who matches on it. The description only says that values which
already failed reported the wrapped number, which does not describe this case.
Could you add this value to the table and mention the change in the description?
##########
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:
**1. [Low] Negative seconds are now reported before an invalid date or hour**
This pre-check changes which field `make_timestamp` reports first when
several inputs are invalid. The description discloses this, and only the ANSI
error text changes (ANSI off still gives NULL). But the order now depends on
the size of the seconds:
| (year, month, day, hour, min, sec) | Before | After |
|---|---|---|
| `(2020, 13, 1, 0, 0, -1)` | `MonthOfYear` | `SecondOfMinute` |
| `(2020, 2, 30, 0, 0, -1)` | `Invalid date 'FEBRUARY 30'` |
`SecondOfMinute` |
| `(2020, 1, 1, 25, 0, -1)` | `HourOfDay` | `SecondOfMinute` |
| `(2020, 13, 1, 0, 0, 61)` | `MonthOfYear` | `MonthOfYear` |
| `(2020, 13, 1, 0, 0, 2147483647)` | `MonthOfYear` | `MonthOfYear` |
| `(2020, 13, 1, 0, 0, 2147483648)` | `MonthOfYear` | `SecondOfMinute` |
The wrap fix itself only needs the whole seconds to fit an `int`. Negative
values inside the `int` range could still go to `LocalDateTime.of`, which
rejected them in field order before this PR, with the negative check staying in
`makeTime`. `make_timestamp` would then report `-1` for `-0.5`, as it did
before this PR.
If the new order is what you want for consistency with `make_time`, that is
fine too, and item 2 would pin it.
##########
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:
**6. [Nit] The method Javadoc still leaves out the seconds**
This comment is anchored here because the Javadoc is above this hunk.
The Javadoc of `makeTimestampMicros` says it throws `SparkDateTimeException`
for an invalid fraction of second and `DateTimeException` for an "invalid
year/month/day/hour/min combination". This call can now throw a plain
`DateTimeException` for the seconds before any other field is looked at, and
`LocalDateTime.of` below still throws one for 61. Could the Javadoc list the
seconds too?
##########
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:
**7. [Nit] The scale assertion guards only one of the two callers of the
split**
The division in `splitSecsAndMicros` is what depends on scale 6, and its
Scaladoc says so, but this assertion stays in one caller and
`DateTimeUtils.makeTime` goes through the split with no check. A wrong scale
would give a silently wrong split: `Decimal(30, 16, 0)` reads as 0 seconds and
30000 nanos.
This is not a live bug, because `MakeTimestamp` and `MakeTime` both declare
`DecimalType(16, 6)`. Moving the check into the split would protect both
callers. A plain `assert(secsAndMicros.scale == 6)` without a message would
also avoid a by-name closure on this per-row path.
##########
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:
**2. [Low] No test pins the order of the checks**
The Scaladoc of `splitSecsAndMicros` and the description state the order of
the checks, but every case in this table has a valid date and time, so moving
the check would not fail any test. With `month = 13`, these would pin it:
```scala
// (sec, the message expected with month = 13)
("-1", "Invalid value for SecondOfMinute (valid values 0 - 59): -1"),
("61", "Invalid value for MonthOfYear (valid values 1 - 12): 13"),
("2147483647", "Invalid value for MonthOfYear (valid values 1 - 12): 13"),
("2147483648", "Invalid value for SecondOfMinute (valid values 0 - 59):
2147483648")
```
##########
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)
Review Comment:
**8. [Nit] The split allocates an object per row**
`makeTimestampMicros` used two `int` locals before and now gets one of these
per row on the code-generated path. The `LocalDateTime.of` call below allocates
as well and escape analysis may remove this one, so the cost is probably small,
and I did not benchmark it.
The split could instead return the checked whole seconds as an `int`, with
each caller computing the nanos. This is not blocking, and keeping the case
class is fine with me.
##########
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:
**4. [Nit] The seven-argument form and the `_ltz` and `_ntz` names get no
new coverage**
The description lists the seven-argument form and all three function names
as fixed. This test only uses `timeZoneId` with `timezone = None`, and the
golden additions only use the six-argument `make_timestamp` and
`try_make_timestamp`. So the `timezone = Some(...)` branch of `nullSafeEval`
and of `doGenCode` never sees these values.
The split runs on the shared path, so the risk is low. A `timezone =
Some(Literal("UTC"))` variant here, and one case each in `timestamp-ltz.sql`
and `timestamp-ntz.sql` next to the existing six-argument calls, would cover
what the description claims.
##########
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:
**5. [Nit] The test name does not match all of its values**
`2147.483647`, `61`, `-0.5` and `-0.000001` all have micros that fit an
`int`, so the name describes only part of the table.
```suggestion
test("SPARK-60743: make_timestamp rejects out-of-range seconds instead of
wrapping them") {
```
--
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]
