spark-test commented on code in PR #59955:
URL: https://github.com/apache/spark/pull/59955#discussion_r4423408150
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1105,6 +1105,34 @@ 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. 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) {
+ throw new DateTimeException(
+ s"Invalid value for SecondOfMinute (valid values 0 - 59):
${secsAndMicros.toLong}")
Review Comment:
**Nit (P3):** This split rejects on the floored whole seconds but prints
`secsAndMicros.toLong`, which truncates towards zero. For a seconds value
strictly between -1 and 0, such as `make_timestamp(2020, 1, 1, 0, 0, -0.5)`,
`Math.floorDiv` gives -1, so the value is rejected, but the message reads
`Invalid value for SecondOfMinute (valid values 0 - 59): 0`. That names a valid
second as the invalid one and hides the input. Before this change,
make_timestamp reported -1 here (through `LocalDateTime.of`). make_time already
printed 0, and routing `makeTimestampMicros` through this helper now carries
that to make_timestamp, make_timestamp_ltz and make_timestamp_ntz with ANSI
mode on. Only the message is affected: the error class and SQLSTATE are right,
and ANSI-off and `try_make_timestamp*` calls still return NULL. The new cases
stop at -4264.967296, so no test covers this range.
One way to fix it: when the given value lies strictly between -1 and 0,
report it as given (for example -0.5), and keep the truncated whole seconds for
every other rejected value. Then update this Scaladoc and the comment in the
new `DateExpressionsSuite` test, which both state the truncation rule. Because
make_time shares the helper, this also corrects its message for the same range.
**Verification:**
- **Regression:** Evaluate make_timestamp with seconds -0.5 and -0.000001
for both TIMESTAMP and TIMESTAMP_NTZ results through interpreted and
code-generated evaluation. With ANSI mode on, assert
DATETIME_FIELD_OUT_OF_BOUNDS.WITHOUT_SUGGESTION with a rangeMessage naming the
given negative value; with ANSI mode off, assert NULL.
- **Regression:** Convert hour, minute and seconds -0.5 with the make_time
conversion and assert the same DATETIME_FIELD_OUT_OF_BOUNDS rangeMessage naming
-0.5.
- **Compatibility:** Confirm that the existing out-of-range seconds
expectations for make_timestamp and make_time (2147.483647 through
9999999999.999999, -4264.967296, -9999999999.999999, -999999999.999999,
4294967297.999999, 61, 60.0 and 100.5), the timestamp SQL golden outputs and
the time SQL golden output for make_time(1, 18, -999999999.999999) keep their
messages and NULL results unchanged.
--
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]
