spark-test commented on code in PR #59955:
URL: https://github.com/apache/spark/pull/59955#discussion_r4465775753
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1105,6 +1105,40 @@ object DateTimeUtils extends SparkDateTimeUtils {
}
}
+ /**
+ * 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,
+ * packed into a long, the seconds in the high 32 bits and the nanos in the
low 32 bits, so
+ * that nothing is allocated per row: `(packed >> 32).toInt` and
`packed.toInt`.
+ *
+ * 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.
Review Comment:
**Nit (P3):** The order of the checks described here is not exact at two
boundaries. The `makeTimestampMicros` Javadoc, the `makeTimestampMicros`
comment that points here, and the order comment in the new
`DateExpressionsSuite` test rely on the same description:
- The split checks the floored whole seconds, not the value. 2147483647.5
has whole seconds `Int.MaxValue`, so it passes the split, and
`make_timestamp(2020, 13, 1, 0, 0, 2147483647.5)` reports `MonthOfYear` first
even though the value is above `Int.MaxValue`. It takes the same path as the
`2147483647` row that the new test pins.
- `makeTimestampMicros` points here for the order of its checks, but it does
not leave every other value to `LocalDateTime.of`. Its 60-second branch runs
first, so `make_timestamp(2020, 13, 1, 0, 0, 60.5)` raises
`INVALID_FRACTION_OF_SECOND` before the month is checked, which also
contradicts the test comment's "any other out-of-range value after it".
Both inputs already behave this way on the base branch, so only the new
documentation is off. Still, the PR presents this order as a pinned contract,
and the new invalid-month rows use only integer seconds, so neither boundary is
exercised.
One way to fix it: here, in the `makeTimestampMicros` Javadoc and in the
test comment, say that the split reports seconds first only when they are
negative or their whole seconds exceed `Int.MaxValue` (2147483648 or more).
Wherever the order is described for `make_timestamp`, also mention the
60-second branch: in the test comment, and in the `makeTimestampMicros`
comments or this Scaladoc. A 2147483647.5 row with month 13 that expects the
`MonthOfYear` message would pin the corrected boundary.
This extends existing discussions
[1](https://github.com/apache/spark/pull/59955#discussion_r4424175152),
[2](https://github.com/apache/spark/pull/59955#discussion_r4424175162).
**Verification:**
- **Behavior:** With ANSI mode on, make_timestamp with month 13 and seconds
2147483647.5 raises DATETIME_FIELD_OUT_OF_BOUNDS with the MonthOfYear message,
while 2147483648 still reports SecondOfMinute first, on both the interpreted
and the code-generated path, for TIMESTAMP with and without a time zone and for
TIMESTAMP_NTZ. With ANSI mode off, both return NULL.
- **Inspection:** Every comment that describes the order of the seconds
checks speaks of whole seconds above Int.MaxValue rather than a value above it,
and the make_timestamp description of that order mentions that a value between
60 and 61 seconds is reported before the other fields. Apart from the added
test row, only comments change.
--
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]
